jansvoboda11 added a comment.
In D102923#2787702 <https://reviews.llvm.org/D102923#2787702>, @dexonsmith
wrote:
> Yup, seems like something that could be added as a follow up (although
> probably using the same remark). Is there a good place to leave behind a
> FIXME?
I'll put a FIXME at an appropriate place in the next revision.
================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:427
+def remark_pp_include_header_search_usage : Remark<
+ "user-provided search path used: '%0'">,
+ InGroup<DiagGroup<"include-header-search-usage">>;
----------------
dexonsmith wrote:
> I suggest, more simply, "search path used:" (drop the "user-provided"). If
> you think it's useful for information purposes to know whether it was a user
> or system search path, I'd suggest using a select (in that case, maybe you
> want to add an optional "framework" descriptor, and/or "header map" when
> applicable, etc.).
I chose this spelling to distinguish user-provided vs default include paths.
The default ones are not important for header search pruning in dependency
scanner, as they always get generated in
`InitHeaderSearch::AddDefaultIncludePaths`.
================
Comment at: clang/include/clang/Lex/HeaderSearch.h:164-165
+ /// Mapping from SearchDir to HeaderSearchOptions::Entry indices.
+ llvm::DenseMap<unsigned, unsigned> SearchDirToHSEntry;
+
----------------
dexonsmith wrote:
> I think this can just be a vector of `unsigned`, since the key is densely
> packed and counting from 0. You can use `~0U` for a sentinel for the
> non-entries. Maybe it's not too important though.
I'm not sure what's our approach on early micro-optimizations like this. I
don't think it will have measurable impact on memory or runtime.
================
Comment at: clang/lib/Frontend/InitHeaderSearch.cpp:581-583
+ // Check whether this DirectoryLookup maps to a HeaderSearch::UserEntry.
+ if (Infos[I].UserEntryIdx)
+ LookupsToUserEntries.insert({I, *Infos[I].UserEntryIdx});
----------------
dexonsmith wrote:
> I don't see why we'd want to filter out system includes.
> - Users sometimes manually configure "system" search paths, and this remark
> is fairly special-purpose, so I'm not sure the distinction is interesting to
> preserve.
> - This remark is already going to be "noisy" and hit a few times on
> essentially every compilation. Adding the system paths that get hit won't
> change that much.
> - The motivation for the patch is to test the logic for detecting which
> search paths are used in isolation from the
> canonicalize-explicit-module-build-commands logic in clang-scan-deps. We need
> to know that the logic works for system search paths as well.
I'm not sure what you mean by system includes.
`HeaderSearchOptions::UserEntries` should contain all search paths provided
through the command-line including `-isystem` and `-isysroot`:
https://github.com/llvm/llvm-project/blob/97d234935f1514af128277943f30efc469525371/clang/lib/Frontend/CompilerInvocation.cpp#L2985-L3056.
"System header prefixes" only control whether a header found through
`DirectoryLookup` should be marked as system.
================
Comment at: clang/test/Preprocessor/header-search-user-entries.c:9-11
+// CHECK: remark: user-provided search path used:
'{{.*}}/header-search-user-entries/a'
+// CHECK: remark: user-provided search path used:
'{{.*}}/header-search-user-entries/a_next'
+// CHECK: remark: user-provided search path used:
'{{.*}}/header-search-user-entries/d'
----------------
dexonsmith wrote:
> If `-verify` doesn't work (hopefully it does), I think you'll need to add
> some `CHECK-NOT` / `CHECK-NEXT` / etc. or else you're not testing for the
> absence of other diagnostics.
>
> Please also test:
> - Framework search path (used vs. not used)
> - System search path (used vs. not used)
> - One search path described relative to `-isysroot` (used vs. not used)
> - Header map (matched and used vs. matched but not used vs. not matched --
> for the middle case, I'm not sure what result we actually want)
> - A path only used via `#if __has_include()` (when it's not followed by an
> `#include` or `#import`)
> - A path only used via ObjC's `#import`
>
> If one of them doesn't work and it's complex enough to separate into a follow
> up, I think that'd be fine, but please find somewhere to leave a FIXME.
Good point, I'll work on improving the coverage.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102923/new/
https://reviews.llvm.org/D102923
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
