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

Remove using namespace std from header files #115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TimmRuppert
Copy link

@TimmRuppert TimmRuppert commented Oct 11, 2024

Reference to a related issue in the repository

Fixes #114

Add a description

Remove using namespace std and add full qualifier where needed

Check the checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation for osi-sensor-model-packaging.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests / Github Actions pass locally with my changes.

@TimmRuppert TimmRuppert self-assigned this Oct 11, 2024
@TimmRuppert TimmRuppert added the quality Quality improvements. label Oct 11, 2024
@TimmRuppert TimmRuppert changed the title remove using namespace std from header files Draft: remove using namespace std from header files Oct 11, 2024
@TimmRuppert TimmRuppert marked this pull request as draft October 11, 2024 12:43
@TimmRuppert TimmRuppert changed the title Draft: remove using namespace std from header files Remove using namespace std from header files Oct 11, 2024
@TimmRuppert TimmRuppert marked this pull request as ready for review October 11, 2024 12:55
@pmai
Copy link
Contributor

pmai commented Oct 11, 2024

I think the much better solution would be to remove the whole header files themselves, since they serve no useful purpose (they are not meant to be included anywhere except the respective sole source file). Then the using statement is in a cpp file and hence unproblematic (it is already unproblematic now, but since people just copy stuff without understanding, it is safer to give them less to copy).

@TimmRuppert
Copy link
Author

I think the much better solution would be to remove the whole header files themselves, since they serve no useful purpose

Yes, that is one option. Personally, I appreciate the improved readability that comes with the split, but I understand that it's a matter of preference. Many users might use the .cpp file as a template for their projects, and I believe that examples in general often serve this purpose as well. However, in that scenario, with or without the headers it should not pose an issue.

Merging this PR would address the immediate problem, and we could consider consolidating everything into a single file as a subsequent step if you prefer that? Perhaps during a broader refactoring and cleanup of the OSMP .cpp files. This could also address minor improvements like making some functions static, handling always-true conditions, initializing all members, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality Quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid using namespace in Header Files
3 participants