GeezerVison

This should be further improved following guidance originally posted by @glankk in #448 (comment)

I suggest we emulate gcc's darwin-specific -F option, and possibly -iframework as well. See https://gcc.gnu.org/onlinedocs/gcc/Darwin-Options.html. The -F paths are interleaved with -I and searched left-to-right, so the order of interleaved normal include paths and frameworks should be preserved. For this we could change the DUI::includePaths from being a vector of path strings to being a vector of structs with a path string and, on darwin, a framework flag that determines what search rules to use.

Looks like this is currently executed for all code even if there is no apple framework is available which seems excessive. I wonder if this behavior should be configurable via DUI or consider the __APPLE__ preprocessor define.

Even if it is available it might not be the files you are looking for (can there be conflicts?) as this behavior would be limited to the Apple compiler.

Looks like this is currently executed for all code even if there is no apple framework is available which seems excessive.

Indeed, this was a deliberate choice anticipating the use of the tool in the context of cross compilation. Adding an option to enable/disable the behavior may be best.

@glankk Thanks for the suggestion 🙏

All the review comments have been addressed. Once the GitHub workflow have been enabled and the test pass, I believe this will be ready for integration 🚀

we also included a comprehensive docstring explaining how to work with the DUI struct.

/**
 * Command line preprocessor settings.
 *
 * Mirrors typical compiler options:
 *   -D <name>=<value>       Add macro definition
 *   -U <name>               Undefine macro
 *   -I <dir>                Add include search directory
 *   -F <dir>                Add framework search directory (Darwin)
 *   -iframework <dir>       Add system framework search directory (Darwin)
 *   --include <file>        Force inclusion of a header
 *   -std=<version>          Select language standard (C++17, C23, etc.)
 *
 * Path search behavior:
 *   - If searchPaths is non-empty, it is used directly, preserving the
 *     left-to-right order and distinguishing between Include, Framework,
 *     and SystemFramework kinds.
 *   - If searchPaths is empty, legacy includePaths is used instead, and
 *     each entry is treated as a normal Include path (for backward
 *     compatibility).
 */

To improve the developer experience, we could also further extend the DUI API:

// Mirrors GCC/Clang -I <dir>
void addIncludePath(const std::string& p) {
    searchPaths.push_back({p, PathKind::Include});
}

// Mirrors GCC/Clang -F <dir>
void addFrameworkPath(const std::string& p) {
    searchPaths.push_back({p, PathKind::Framework});
}

// Mirrors GCC/Clang -iframework <dir>
void addSystemFrameworkPath(const std::string& p) {
    searchPaths.push_back({p, PathKind::SystemFramework});
}

If that sounds reasonable, I can amend the last commit and update the tests.

cc: @danmar

Waiting this is integrated, we will stage those changes into a fork and move forward with vendoring those into PythonQt.

Related:

• ENH: Improve Qt include path resolution macOS MeVisLab/pythonqt#271

#283 possibly needs to be addressed as prerequisite of this as the include directories are currently not differentiated between system and "local" ones.

@jcfr Thank you for the comprehensive solution. I am closing #448.

Thanks again @hjmjohnson for working on the initial patch and establishing the momentum 🙏

Hopefully our contributions will be integrated shortly 🤞

please look at adding such helper functions. It sounds good to me.

I will add those

re: support for -system

I am also adding support for this along with integration test update. This issue is related:

• differentiate local includes and system includes #283

@danmar

So if I understand it correctly.. if the command line only uses -I then behavior will be unchanged?

Ditto 👍 Both openHeader (via simplecpp::preprocess) and FileDataCache::tryload (via FileDataCache::get) behave identically whether only -I is passed on the CLI or includePaths are provided directly through simplecpp::DUI.

this looks pretty good to me. I would like a review by @glankk

🙏 This is now finalized and ready for review. The PR description has been updated to reflect the full set of changes 🚀

To improve the developer experience, we could also further extend the DUI API:

please look at adding such helper functions. It sounds good to me.

Done. Convenience helpers for addIncludePath, addSystemIncludePath, addFrameworkPath, and addSystemFrameworkPath are included.

I also made sure that each commit in the series compiles and passes the tests individually. Given that, I'd suggest merging with Rebase & Merge (or Merge) to preserve the history, rather than Squashing & Merge. If you'd prefer to consolidate some of the commits for a cleaner log, let me know I would be happy to revisit.

cc: @glankk

Not to throw a wrench into this but I think #475 and #524 should land/be fixed before this is being merged. Otherwise things could get quite messy down the road. We might also need to merge those other things downstream first as those regressions are serious and we might even need to backport them.

it sounds like the simplecpp repo is not fully ready for this PR yet. but I hope we can make the repo ready and get this PR into simplecpp repo soonish and then make a simplecpp release so we can upstream it into cppcheck and test it with daca etc.

I think this looks pretty good overall, the only thing that bothers me a little is building all of the candidate path strings before they're known to be needed. We've recently made an effort to improve the include search performance, I'd prefer if the path strings are constructed as they're searched. I don't know how much this will actually affect the performance though, maybe it's negligible.

I think I agree with @firewave that fixing #475 and #524 first would be good. I'm working on #524 now.

I don't know how much this will actually affect the performance though, maybe it's negligible.

After #438 has been merged and I will extend the callgrind step with that and that might give an indication of the performance impact (so more to merge beforehand?).

but I hope we can make the repo ready and get this PR into simplecpp repo soonish and then make a simplecpp release so we can upstream it into cppcheck and test it with daca etc.

As mentioned above we probably need to release a version to downstream before this is merged so we can prepare yet another patch (which also needs some test-related backports which have been requested by a packager).

In #438 I encountered a framework path. I only treat it like a regular include path but after that has been merged we should hook it up properly via the options you introduced. Might also double as an integration test.

I think we need to get this into simplecpp soon before it starts to rot.. there are conflicts now.

Thanks for the update 🙏. I may be able to rebase & address conflict within a week

I think we should get the MinGW changes into a release first and get it downstream and do an immediately release with these changes. It is already a lot of changes and might help with bisection.

goal: try to make sure that performance is not affected negatively by this PR if we only provide -I flags as before..

I have WIP changes for selfcheck.sh which runs it with callgrind and will help with this. Since I am currently out sick I cannot work on this right now.

I think we should get the MinGW changes into a release first and get it downstream and do an immediately release with these changes. It is already a lot of changes and might help with bisection.

I don't see why a mingw release would be needed first?

I fear the mingw was mentioned as a prerequisite in august and there is still not mingw available .. imho if prerequisites are not added soon I think we should merge this pr anyway.

I have WIP changes for selfcheck.sh which runs it with callgrind and will help with this. Since I am currently out sick I cannot work on this right now.

well it would be nice with some callgrind scripts.. but I do hope you rest and take care and I am not trying to push you to work even harder.. I believe we can use callgrind anyway to see the impact of this PR even if we don't have a script

I don't see why a mingw release would be needed first?

Since it exposed major bugs - it should have been ported back to 2.18 but there were just too many issues (it just exposed another one in the improved isAbsolutePath()).

well it would be nice with some callgrind scripts

There already is one but it does not rely on include paths as the additional selfcheck does.

I believe we can use callgrind anyway to see the impact of this PR even if we don't have a script

Just add valgrind --tool=callgrind to the ./simplecpp calls in selfcheck.sh. I have it parameterized and such and it just needs to be cleaned up so not much work is necessary I just cannot be bothered right now cough.

I don't see why a mingw release would be needed first?
Since it exposed major bugs - it should have been ported back to 2.18 but there were just too many issues (it just exposed another one in the improved isAbsolutePath()).

People are waiting for the new simplecpp. It solves critical performance issues for a big customer.

I will tag simplecpp now so we can merge simplecpp to cppcheck.. and I feel that if mingw is not added soonish then let's merge this PR without it.

I will tag simplecpp now so we can merge simplecpp to cppcheck.. and I feel that if mingw is not added soonish then let's merge this PR without it.

The previous MinGW issues are all fixed but the absolute path tests exposed new issues - see #556. It is probably an easy fix but I need to do some manual testing to determine the current behavior and on how to test it properly in the CI. Since my cold has actually worsened I have not been able to look into it yet.

well it would be nice with some callgrind scripts

There already is one but it does not rely on include paths as the additional selfcheck does.

I believe we can use callgrind anyway to see the impact of this PR even if we don't have a script

Just add valgrind --tool=callgrind to the ./simplecpp calls in selfcheck.sh. I have it parameterized and such and it just needs to be cleaned up so not much work is necessary I just cannot be bothered right now cough.

This is ready for review in #560.

Once #560 is integrated, I will rebase this pull request.

Once #560 is integrated, I will rebase this pull request.

It has been merged.