Skip to content

Improve portability #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PDLdeLange opened this issue Mar 30, 2021 · 7 comments · Fixed by #95
Closed

Improve portability #88

PDLdeLange opened this issue Mar 30, 2021 · 7 comments · Fixed by #95
Assignees
Labels
enhancement New feature or request

Comments

@PDLdeLange
Copy link
Contributor

We love the effort but currently we can not use the project due to portability issues. I would propose to make the following changes.

  • Introduce "casbin" namespace.
  • Remove "using namespace" in all headers.
  • Remove "using std" everywhere.

The latter two changes conflict with point 3 of the contribution guide which is actually considered bad practice and decreases portability. See: https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice.

Making these changes would make the project more inline with point 12 of the contributation guide and would improve portablity. See also: https://google.github.io/styleguide/cppguide.html#Namespaces.

Happy to help out on this in case this issue is accepted.

@EmperorYP7
Copy link
Member

EmperorYP7 commented Mar 30, 2021

Introduce "casbin" namespace.

I agree. This is really something we should be incorporating here.

Remove "using namespace"/"using std" in all headers

Apart from that, some parts of the directory structure uses snake case which might create unnecessary conflicts with STL. Here's a nice resource where I got to know about this first.

@hsluoyz hsluoyz self-assigned this Mar 30, 2021
@hsluoyz hsluoyz added the enhancement New feature or request label Mar 30, 2021
@hsluoyz
Copy link
Member

hsluoyz commented Mar 30, 2021

@xcaptain

@bokket
Copy link

bokket commented Mar 30, 2021

引入"卡斯宾"命名空间。

我同意。这确实是我们应该在这里纳入的东西。

删除所有标题中的"使用命名空间"/"使用 std"

除此之外,目录结构的某些部分使用蛇壳,这可能会与 STL 产生不必要的冲突。这里有一个很好的资源,在那里我首先了解了这一点。

Amazing! we are following the same YouTube blogger!

@bokket
Copy link

bokket commented Mar 30, 2021

We love the effort but currently we can not use the project due to portability issues. I would propose to make the following changes.

  • Introduce "casbin" namespace.
  • Remove "using namespace" in all headers.
  • Remove "using std" everywhere.

The latter two changes conflict with point 3 of the contribution guide which is actually considered bad practice and decreases portability. See: https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice.

Making these changes would make the project more inline with point 12 of the contributation guide and would improve portablity. See also: https://google.github.io/styleguide/cppguide.html#Namespaces.

Happy to help out on this in case this issue is accepted.

I think cross-platform testing is the first need, it is very unfriendly to Linuxers like me (wry smiles)

@xcaptain
Copy link

Agree, we should close #7 and can simply consider using google's style.

@hsluoyz
Copy link
Member

hsluoyz commented Mar 31, 2021

@PDLdeLange I think your opinions sound good. Go ahead!

@PDLdeLange
Copy link
Contributor Author

PDLdeLange commented Mar 31, 2021 via email

PDLdeLange added a commit to PDLdeLange/casbin-cpp that referenced this issue Apr 3, 2021
Removed 'using namespace std', introduced casbin namespace.

Signed-off-by: PDLdeLange <[email protected]>
PDLdeLange added a commit to PDLdeLange/casbin-cpp that referenced this issue Apr 3, 2021
Signed-off-by: PDLdeLange <[email protected]>

Fix for casbin#88.

Description:
- removed 'using namespace std'
- introduced casbin namespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants