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

fix: always use absolute (and simplified) header path whenever possible #362

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

Conversation

Tal500
Copy link

@Tal500 Tal500 commented Aug 6, 2024

This fix an issue of importing the same header by different addresses.
The issue is trickier than seems, because if a user used the include dir flag -I... with an absolute path, then every matched header was detected to be absolute, but if the import was a relative import to the source file and the source file isn't with an absolute path, than the header is identified by the combined relative path.

See https://sourceforge.net/p/cppcheck/discussion/general/thread/c01f80556b/

Note - I tried to make the code compatible with Windows, but was tested only on linux.

This fix an issue of importing the same header by different addresses.
The issue is trickier than seems, because if a user used the include dir flag `-I...` with an absolute path, then every matched header was detected to be absolute, but if the import was a relative import to the source file and the source file isn't with an absolute path, than the header is identified by the combined relative path.
See https://sourceforge.net/p/cppcheck/discussion/general/thread/c01f80556b/
@firewave
Copy link
Collaborator

firewave commented Aug 6, 2024

This change might be problematic because it is obviously requires a test but so far there are no tests at all utilizing includes (see #261 for a related discussion).

This should probably be implemented by introducing Python tests similar to what Cppcheck is doing. @danmar would that be acceptable for tests which actually require (multiple) files?

@Tal500
Copy link
Author

Tal500 commented Aug 6, 2024

This change might be problematic because it is obviously requires a test but so far there are no tests at all utilizing includes (see #261 for a related discussion).

This should probably be implemented by introducing Python tests similar to what Cppcheck is doing. @danmar would that be acceptable for tests which actually require (multiple) files?

What about these (failed) tests? https://ci.appveyor.com/project/danmar/simplecpp/builds/50353854

@danmar
Copy link
Owner

danmar commented Aug 12, 2024

This should probably be implemented by introducing Python tests similar to what Cppcheck is doing. @danmar would that be acceptable for tests which actually require (multiple) files?

sure, it would definitely make sense to include actual files in some python tests but as @Tal500 pointed out there are some tests for the handling at least.

What about these (failed) tests?

yes imho those are valuable also.

@Tal500
Copy link
Author

Tal500 commented Aug 14, 2024

@danmar if I guess correctly, the failure is due to that the header files are not really in the working directory or so, right?

@danmar
Copy link
Owner

danmar commented Aug 16, 2024

if I guess correctly, the failure is due to that the header files are not really in the working directory or so, right?

yes that sounds probable to me.

I don't know how your changes will affect the output from simplecpp if all filepaths will always be absolute that is also unfortunate. Is the output changed?

@Tal500
Copy link
Author

Tal500 commented Aug 18, 2024

I don't know how your changes will affect the output from simplecpp if all filepaths will always be absolute that is also unfortunate. Is the output changed?

Yes, except for the system headers that were not found in the scan paths, e.g. STL headers (that simplecpp doesn't open them at all and it's not consider to be an error).

We could have a more complicated solution, that uses two header attributes:

  1. Name: The (first) included path, perhaps normalized
  2. Path: The absolute path that it is loaded from

The first attribute is mandatory, and the second one is optional and exists only when the header is found.
That way, the error messages could still be looking the same as for today, but still correct.

An alternative solution is to have a user configuration that toggles the include path absoluteness.

@Tal500
Copy link
Author

Tal500 commented Sep 4, 2024

We could have a more complicated solution, that uses two header attributes:
@danmar what do you think about this solution? Do you have a better idea?

@danmar
Copy link
Owner

danmar commented Sep 28, 2024

We could have a more complicated solution, that uses two header attributes:
@danmar what do you think about this solution

I am very sorry for the delay. I was at cppcon last week and had a presentation.

As far as I understand the solution you suggest makes sense to me. We don't need to have these properties for each token right.

@danmar
Copy link
Owner

danmar commented Sep 28, 2024

@Tal500 just making sure you see my comment.

@Tal500
Copy link
Author

Tal500 commented Sep 29, 2024

I am very sorry for the delay. I was at cppcon last week and had a presentation.

I'd like too see it when it will be published!

As far as I understand the solution you suggest makes sense to me. We don't need to have these properties for each token right.

I'll look into that more

@danmar
Copy link
Owner

danmar commented Sep 29, 2024

I am very sorry for the delay. I was at cppcon last week and had a presentation.

I'd like too see it when it will be published!

👍

for your information here is a short description:
https://cppcon2024.sched.com/event/1gZdy/building-cppcheck-what-we-learned-from-17-years-of-development

I guess it will take a while until it is published.

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