Skip to content

Conversation

jcfr
Copy link
Contributor

@jcfr jcfr commented Aug 26, 2025

This PR modernizes simplecpp's header lookup to more closely match GCC/Clang, while preserving backward compatibility for existing users.

What's new

  • Typed, ordered search paths

    • Introduce DUI::SearchPath with PathKind:

      • Include (for -I)
      • SystemInclude (for -isystem)
      • Framework (for -F, Darwin)
      • SystemFramework (for -iframework, Darwin)
    • If DUI::searchPaths is non-empty, it is honored verbatim in the order provided.
      If it's empty, legacy includePaths are mirrored as Include entries for back-compat.

  • CLI flags

    • Add support for -F<dir> and -iframework<dir> (framework lookups).
    • Add support for -isystem<dir> (system include lookups).
    • Keep -I<dir> as before.
  • Public API convenience

    • New helpers on DUI:

      • addIncludePath(path, legacy=false)
      • addSystemIncludePath(path)
      • addFrameworkPath(path)
      • addSystemFrameworkPath(path)
    • addIncludePath defaults to legacy=false; tests exercise both modes.

  • Darwin frameworks

    • Framework includes like <Pkg/Hdr.h> are resolved to Pkg.framework/{Headers,PrivateHeaders}/Hdr.h.
    • Implement toAppleFrameworkRelatives() returning prioritized candidates (Headers first, then PrivateHeaders).
    • Works for both __has_include and normal #include.
  • Lookup order (summary of actual implementation)

    1. For "quotes" includes only: directory of the including file (unchanged).

    2. Interleaved searchPaths in left-to-right CLI order:

      • Include (-I)
      • Framework (-F) — using the Headers/PrivateHeaders rewrite when applicable
    3. System include paths (SystemInclude, i.e., -isystem)

    4. System framework paths (SystemFramework, i.e., -iframework)

    5. (Standard library dirs are outside simplecpp's scope; -idirafter/-iquote are not implemented in this series.)

  • Core loader refactors

    • openHeader() and FileDataCache::get() now consult typed, ordered paths and produce candidates accordingly.
    • When resolving framework headers, both Headers and PrivateHeaders are tried in that order.

Tests & tooling

  • Integration tests

    • New helpers to build fixtures:

      • generic headers, sources, and minimal framework trees.
    • test_framework_lookup: verifies Headers vs PrivateHeaders and -F vs -iframework.

    • test_searchpath_order: a single parametrized test that validates precedence across -I, -isystem, -F, -iframework, including interleaving and duplicates, asserting on the exact #line path chosen.

  • Unit tests (test.cpp)

    • Framework handling covered for both #include and __has_include.
    • Back-compat exercised by running key tests twice (new ordered API and legacy mode via addIncludePath(..., /*legacy=*/true)).

Supersedes the following pull request:


Related issues:

@jcfr
Copy link
Contributor Author

jcfr commented Aug 26, 2025

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.

@firewave
Copy link
Collaborator

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.

@jcfr
Copy link
Contributor Author

jcfr commented Aug 26, 2025

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.

@jcfr jcfr force-pushed the apple-framework-header-support branch 2 times, most recently from c5a5cbe to cdd5fc2 Compare August 26, 2025 17:54
@jcfr
Copy link
Contributor Author

jcfr commented Aug 26, 2025

@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

@jcfr
Copy link
Contributor Author

jcfr commented Aug 26, 2025

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

Related:

@firewave
Copy link
Collaborator

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

@hjmjohnson
Copy link

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

@jcfr
Copy link
Contributor Author

jcfr commented Aug 27, 2025

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

Hopefully our contributions will be integrated shortly 🤞

@jcfr

This comment was marked as outdated.

@jcfr jcfr force-pushed the apple-framework-header-support branch from e0ed0e3 to 3a900a2 Compare August 29, 2025 05:05
@jcfr jcfr changed the title Add Apple Framework header support for __has_include and #include feat: Add Darwin-style framework search with ordered path lookup Aug 29, 2025
@jcfr jcfr force-pushed the apple-framework-header-support branch from 3a900a2 to 6899f8e Compare August 29, 2025 05:25
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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.

@jcfr
Copy link
Contributor Author

jcfr commented Aug 29, 2025

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:

@jcfr jcfr force-pushed the apple-framework-header-support branch 2 times, most recently from 267c55c to 2af5465 Compare August 30, 2025 04:20
@jcfr jcfr changed the title feat: Add Darwin-style framework search with ordered path lookup feat: Ordered search paths for -I, -isystem, -F, -iframework + Darwin framework support (with legacy -I back-compat) Aug 30, 2025
@jcfr jcfr force-pushed the apple-framework-header-support branch from 2af5465 to de861c9 Compare August 30, 2025 05:09
@jcfr
Copy link
Contributor Author

jcfr commented Aug 30, 2025

@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

jcfr and others added 5 commits August 30, 2025 15:30
…th lookup

This change teaches simplecpp to resolve headers from Apple-style Framework
directories while preserving the left-to-right order of interleaved
-I/-F/-iframework search paths (like GCC/Clang on Darwin).

This enables both:
- `__has_include(<Pkg/Hdr.h>)` -> `<Pkg.framework/Headers/Hdr.h>` (or `PrivateHeaders`)
- `#include <Pkg/Hdr.h>`       -> same framework layout when a package prefix exists

Changes:
- Add `DUI::SearchPath` with `PathKind {Include, Framework, SystemFramework}`.
- If `DUI::searchPaths` is non-empty, use it verbatim (interleaved -I/-F/-iframework).
  Otherwise preserve back-compat by mirroring `includePaths` as Include paths.
- Update `openHeader()` to consult typed paths, and only rewrite `<Pkg/Hdr.h>`
  to `Pkg.framework/{Headers,PrivateHeaders}/Hdr.h` when a package prefix exists.
- Implement `toAppleFrameworkRelatives()` returning prioritized candidates
  (Headers first, then PrivateHeaders).
- Tests use `PathKind::Framework` when checking framework layout.

CLI
- Support -F<dir> and -iframework<dir> (keep -I as before).

Behavior notes
- The order of -I/-F/-iframework is preserved exactly as provided.
- `Framework` vs `SystemFramework` differ only in diagnostic semantics (not lookup).
- Legacy users who only set `DUI::includePaths` see identical behavior.

Tests
- Add `appleFrameworkHasIncludeTest` for `__has_include` resolution.
- Add `appleFrameworkIncludeTest` for `#include` resolution.
- Add dummy fixture: `testsuite/Foundation.framework/Headers/Foundation.h`.

This brings simplecpp closer to GCC/Clang behavior on macOS and enables
robust resolution of framework headers like `Foundation/Foundation.h`.

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
Co-authored-by: Hans Johnson <hans-johnson@uiowa.edu>
Suggested-by: glankk <glankk@users.noreply.github.com>
This change introduces support for the -isystem flag, allowing users to
specify system include directories. The addition includes handling the
new flag in both the argument parsing and include resolution logic,
as well as updating relevant tests to validate the new functionality.
jcfr added 2 commits August 30, 2025 15:42
This improves test coverage by ensuring that DUI::addIncludePath is
tested with both legacy mode on and off. It adds new test functions
for each scenario and updates the test cases accordingly.
This changes the default value of the 'legacy' parameter in the
DUI::addIncludePath method to false. Since this API was just introduced,
it is preferred to use the new capabilities by preserving the left-to-right
order.
@firewave
Copy link
Collaborator

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.

@danmar
Copy link
Owner

danmar commented Sep 1, 2025

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.

@glankk
Copy link
Contributor

glankk commented Sep 1, 2025

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.

@firewave
Copy link
Collaborator

firewave commented Sep 1, 2025

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).

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.

differentiate local includes and system includes
5 participants