dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed.
Thanks for your patience! I thought I already looked at this last week. This is looking close. Comments mostly inline, but a high level point: I think this should report system search paths. In D102923#2787001 <https://reviews.llvm.org/D102923#2787001>, @jansvoboda11 wrote: > In D102923#2781386 <https://reviews.llvm.org/D102923#2781386>, @Bigcheese > wrote: > >> I also wonder how we should handle other things that are found via include >> paths such as module maps. > > That's a good point. The logic for module map lookup is more complex than > header lookup, so I'd be keen to tackle that in a follow-up patch. 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? ================ 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">>; ---------------- 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.). ================ Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:428 + "user-provided search path used: '%0'">, + InGroup<DiagGroup<"include-header-search-usage">>; def err_pp_file_not_found_angled_include_not_fatal : Error< ---------------- I think it'd be better to define a DiagGroup in clang/include/clang/Basic/DiagnosticGroups.td and use it here. It might be nice for it to be more general, and not refer to headers, so we can reuse it for module maps, and/or potentially other remarks. Maybe, `-Rsearch-paths`? ================ Comment at: clang/include/clang/Lex/HeaderSearch.h:164-165 + /// Mapping from SearchDir to HeaderSearchOptions::Entry indices. + llvm::DenseMap<unsigned, unsigned> SearchDirToHSEntry; + ---------------- 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. ================ 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}); ---------------- 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. ================ Comment at: clang/test/Preprocessor/header-search-user-entries.c:1-4 +// RUN: %clang_cc1 -fsyntax-only %s -Rinclude-header-search-usage \ +// RUN: -I%S/Inputs/header-search-user-entries/a -I%S/Inputs/header-search-user-entries/a_next \ +// RUN: -I%S/Inputs/header-search-user-entries/b -I%S/Inputs/header-search-user-entries/c \ +// RUN: -I%S/Inputs/header-search-user-entries/d 2>&1 | FileCheck %s ---------------- Can we use `-verify` here? Or does that not work for remarks? ================ 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' ---------------- 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. 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
