Skip to content
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

chore: Remove anonymous namespace from GeneralUtils.h #1460

Merged
merged 3 commits into from
Feb 24, 2024
Merged

Conversation

jadebenn
Copy link
Collaborator

@jadebenn jadebenn commented Feb 21, 2024

Anonymous namespaces in header files do not actually prevent access to the enclosed functions in corresponding .cpp files and having one here is therefore pointless

EmosewaMC
EmosewaMC previously approved these changes Feb 21, 2024
Copy link
Collaborator

@EmosewaMC EmosewaMC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively this could just be moved to a namespace called priv, that way Parse_ doesn't show up in hints. though it would be preferable to have the hidden function be called _Parse as we have done elsewhere in GeneralUtils

@jadebenn
Copy link
Collaborator Author

jadebenn commented Feb 21, 2024

alternatively this could just be moved to a namespace called priv, that way Parse_ doesn't show up in hints. though it would be preferable to have the hidden function be called _Parse as we have done elsewhere in GeneralUtils

Any identifier with a leading underscore is technically reserved by the standard, which is why I opted for a trailing underscore instead.

@jadebenn
Copy link
Collaborator Author

jadebenn commented Feb 21, 2024

Also, since these are explicit template instantiations, I think it should be possible to move them to the cpp file and hide the implementation functions that way, so I might try that.

@jadebenn
Copy link
Collaborator Author

Actually, no, I don't think that would work. The TryParse function is not explicitly instantiated and would need access to Parse_ in the function body, so even if I moved Parse_ to the source file, I'd still need to provide a declaration in the header, meaning it would still be accessible through any included header file. I don't think I can move TryParse out either, though perhaps that could be achieved via extern template (which I understand poorly).

@EmosewaMC
Copy link
Collaborator

EmosewaMC commented Feb 21, 2024

alternatively this could just be moved to a namespace called priv, that way Parse_ doesn't show up in hints. though it would be preferable to have the hidden function be called _Parse as we have done elsewhere in GeneralUtils

Any identifier with a leading underscore is technically reserved by the standard, which is why I opted for a trailing underscore instead.

You're thinking of double underscore. Single underscore is fine so long as a capital letter does not follow.

@jadebenn
Copy link
Collaborator Author

alternatively this could just be moved to a namespace called priv, that way Parse_ doesn't show up in hints. though it would be preferable to have the hidden function be called _Parse as we have done elsewhere in GeneralUtils

Any identifier with a leading underscore is technically reserved by the standard, which is why I opted for a trailing underscore instead.

You're thinking of double underscore.

They're both reserved in C++ according to what I've read: https://devblogs.microsoft.com/oldnewthing/20230109-00/?p=107685

@EmosewaMC
Copy link
Collaborator

That page is talking about global scope which does not apply here.

@jadebenn
Copy link
Collaborator Author

jadebenn commented Feb 21, 2024

No, though I was mistaken in my recollection to you. Identifiers with a leading underscore and a capital letter are reserved everywhere. So _parse would be fine outside of global scope, but _Parse would technically be a standard violation (even though it would probably compile and run without issue).

@jadebenn jadebenn requested a review from EmosewaMC February 22, 2024 02:13
@EmosewaMC
Copy link
Collaborator

How has this been tested?

@jadebenn
Copy link
Collaborator Author

jadebenn commented Feb 22, 2024

I've compiled with the #ifdefs commented out and tried the game a little. I haven't done much because none of the underlying logic has been touched, so I didn't think more than a cursory check was necessary.

@EmosewaMC
Copy link
Collaborator

Code at the very least should be run and tested in some capacity, either through unit or integration, as untested code paths may cause issues during runtime; just good practice to hop in game and hit your code path a bit or write a unit test

@jadebenn jadebenn merged commit 5ae8fd8 into main Feb 24, 2024
4 checks passed
@jadebenn jadebenn deleted the noAnonNamespace branch February 24, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants