-
Notifications
You must be signed in to change notification settings - Fork 94
feat: Ordered search paths for -I
, -isystem
, -F
, -iframework
+ Darwin framework support (with legacy -I
back-compat)
#511
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
base: master
Are you sure you want to change the base?
Conversation
This should be further improved following guidance originally posted by @glankk in #448 (comment)
|
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 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. |
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. |
c5a5cbe
to
cdd5fc2
Compare
@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
To improve the developer experience, we could also further extend the
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 Related: |
#283 possibly needs to be addressed as prerequisite of this as the include directories are currently not differentiated between system and "local" ones. |
Thanks again @hjmjohnson for working on the initial patch and establishing the momentum 🙏 Hopefully our contributions will be integrated shortly 🤞 |
39eeb91
to
e0ed0e3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
e0ed0e3
to
3a900a2
Compare
__has_include
and #include
3a900a2
to
6899f8e
Compare
There was a problem hiding this 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.
I will add those
I am also adding support for this along with integration test update. This issue is related: |
267c55c
to
2af5465
Compare
-I
, -isystem
, -F
, -iframework
+ Darwin framework support (with legacy -I
back-compat)
2af5465
to
de861c9
Compare
Ditto 👍 Both
🙏 This is now finalized and ready for review. The PR description has been updated to reflect the full set of changes 🚀
Done. Convenience helpers for 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 |
…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.
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.
de861c9
to
03d7ab0
Compare
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. |
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?).
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). |
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
withPathKind
: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 asInclude
entries for back-compat.CLI flags
-F<dir>
and-iframework<dir>
(framework lookups).-isystem<dir>
(system include lookups).-I<dir>
as before.Public API convenience
New helpers on
DUI
:addIncludePath(path, legacy=false)
addSystemIncludePath(path)
addFrameworkPath(path)
addSystemFrameworkPath(path)
addIncludePath
defaults tolegacy=false
; tests exercise both modes.Darwin frameworks
<Pkg/Hdr.h>
are resolved toPkg.framework/{Headers,PrivateHeaders}/Hdr.h
.toAppleFrameworkRelatives()
returning prioritized candidates (Headers first, then PrivateHeaders).__has_include
and normal#include
.Lookup order (summary of actual implementation)
For
"quotes"
includes only: directory of the including file (unchanged).Interleaved
searchPaths
in left-to-right CLI order:Include
(-I
)Framework
(-F
) — using the Headers/PrivateHeaders rewrite when applicableSystem include paths (
SystemInclude
, i.e.,-isystem
)System framework paths (
SystemFramework
, i.e.,-iframework
)(Standard library dirs are outside simplecpp's scope;
-idirafter
/-iquote
are not implemented in this series.)Core loader refactors
openHeader()
andFileDataCache::get()
now consult typed, ordered paths and produce candidates accordingly.Headers
andPrivateHeaders
are tried in that order.Tests & tooling
Integration tests
New helpers to build fixtures:
test_framework_lookup
: verifiesHeaders
vsPrivateHeaders
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
)#include
and__has_include
.addIncludePath(..., /*legacy=*/true)
).Supersedes the following pull request:
Related issues: