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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
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">>;
----------------
jansvoboda11 wrote:
> 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`.
Ah, I thought that logic was all moved to the driver. Looking at AddDefaultIncludePaths, you're right that only some things have moved, and some haven't (yet)...

For example, it looks like the only logic remaining in `-cc1` on Darwin is:
```
  // All header search logic is handled in the Driver for Darwin.
  if (triple.isOSDarwin()) {
    if (HSOpts.UseStandardSystemIncludes) {
      // Add the default framework include paths on Darwin.
      AddPath("/System/Library/Frameworks", System, true);
      AddPath("/Library/Frameworks", System, true);
    }
    return;
  }
```
(We should probably clean that up and move it to the driver as well...)

I think `-cc1` can be agnostic about the source of the search path (user, IDE, driver, etc.).


================
Comment at: clang/include/clang/Lex/HeaderSearch.h:164-165
 
+  /// Mapping from SearchDir to HeaderSearchOptions::Entry indices.
+  llvm::DenseMap<unsigned, unsigned> SearchDirToHSEntry;
+
----------------
jansvoboda11 wrote:
> 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.
Heh, maybe with the principle that the small things add up (or maybe just out of habit), I probably tend too much toward making small things efficient when it's easy. I also personally find `Vector<Optional>` just as natural a map data structure as `DenseMap`, but with more obvious allocation / etc. But yeah, probably not important.


================
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});
----------------
jansvoboda11 wrote:
> 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.
Thanks for explaining; you're right, I was confused. (Maybe this should be renamed from UserEntries to CommandLineEntries if it has everything on the command-line?)

As I mentioned above, the distinction between driver-generated and `-cc1`-generated options is a bit murky (and moving in the direction of removing the latter), so I'm not sure it's meaningful to filter out `-cc1`-generated options.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102923/new/

https://reviews.llvm.org/D102923