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

Comment and code contradict each other #77

Closed
NOhs opened this issue Aug 14, 2019 · 5 comments · Fixed by #76
Closed

Comment and code contradict each other #77

NOhs opened this issue Aug 14, 2019 · 5 comments · Fixed by #76
Labels
discussion Something that might end up being labled enhancement or wontfix

Comments

@NOhs
Copy link
Collaborator

NOhs commented Aug 14, 2019

Looking at the following code:

# Header only include directories are always public
if target.__class__ is HeaderOnly:
self.include_directories += target.include_directories
self.include_directories += target.include_directories_public

We see that while the comment says that header only targets' include directories are always public, this is not realized in the actual code, hence requiring this unnecessary check to then also include the "non-public" include directories of header only targets.

@NOhs NOhs added the discussion Something that might end up being labled enhancement or wontfix label Aug 14, 2019
@NOhs NOhs changed the title Comment in code and code contradict each other Comment and code contradict each other Aug 14, 2019
@GPMueller
Copy link
Contributor

You mean the headeronly target should not have any flags in the non-public member variable? The comment refers to the behaviour the user should experience.

@NOhs
Copy link
Collaborator Author

NOhs commented Aug 14, 2019

The header only target should have all include_directories in the self.include_directories_public so it behaves as the user experiences it and does not require some extra treatment. It is not a bug, more like a design issue imho.

@GPMueller
Copy link
Contributor

Agreed, this would be better. There might be some places in the init that need changing, I think some defaults are placed in those variables for all target kinds.

@NOhs
Copy link
Collaborator Author

NOhs commented Aug 14, 2019

I am currently trying to get an overview of the init function, I think it can be simplified a bit and at the same time the above mentioned can be implemented.

@NOhs
Copy link
Collaborator Author

NOhs commented Aug 14, 2019

Should be "fixed" with #76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Something that might end up being labled enhancement or wontfix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants