search

danmar / simplecpp

C++ preprocessor
BSD Zero Clause License
204 stars 80 forks source link

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

Open Tal500 opened 1 month ago

Tal500 commented 1 month ago

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.

firewave commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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 commented 14 hours ago

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 commented 14 hours ago

@Tal500 just making sure you see my comment.