[PATCH] D102923: [clang][lex] Remark for used header search paths

[Jan Svoboda via Phabricator via cfe-commits]

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