[PATCH] D155878: [clangd] Loose include-cleaner matching for verbatim headers

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 4 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Headers.h:179
+  // All paths are canonical (FileManager::getCanonicalPath()).
+  std::vector<std::string> SearchPathCanonical;
+
----------------
kadircet wrote:
> s/SearchPathCanonical/SearchPathsCanonical/
Huh, I'd always thought of "search path" singular, like each entry is edges along a path, which makes no sense.
Maybe it's path like a reference to $PATH, which has a singular name for whatever reason.

Anyway, done.


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:486-491
+  include_cleaner::walkUsed(AST.getLocalTopLevelDecls(), {},
+                            AST.getPragmaIncludes(), AST.getSourceManager(),
+                            [&](const include_cleaner::SymbolReference &Ref,
+                                llvm::ArrayRef<include_cleaner::Header> P) {
+                              Providers[llvm::to_string(Ref.Target)] = P.vec();
+                            });
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > i don't think making this test rely on `walkUsed` is a good idea. what about just populating the providers directly, like:
> > > 
> > > ```
> > > EXPECT_TRUE(isPreferredProvider(Decl, Includes, {include_cleaner::Header{"decl.h"}, include_cleaner::Header{"def.h"}}));
> > > ```
> > > 
> > > and verifying we're recognizing preferred providers purely on that ordering ?
> > That would be a strict unit test, but I think it's too easy to lose track of how this idea of "preferred" relates to the code.
> > 
> > For example, when writing this test, I expected `Y` to match the includes for both `Decl` and `Def`, because they're intuitively both "best matches" and we do allow multiple includes to match.
> > But of course we force the *providers* to be ranked, so really ~only multiple includes of the *same* header will be accepted as all-preferred.
> > 
> > This wrinkle seems worth recording, and totally disappears if we make the test abstract by describing a provider list directly.
> > 
> > (Can do it though if that's what you do prefer)
> i completely agree with what you said, but I don't see why ranking should matter here. i do feel like all of those concerns you raised can still be inferred from the fact that we're providing a set of ordered providers, but maybe it's just me. i just feel like the downside is we should update this test if ranking for providers changes for whatever reason, and i don't think we should. but that is not a big deal, so feel free to keep it as-is.
> i completely agree with what you said, but I don't see why ranking should matter here.

I guess I'm basically saying that this function does something that seems reasonable, and the ranking function does something that seems reasonable, but the result when you plug them together is surprising.

That makes me want to write a test that has them plugged together, but I don't feel strongly. Changed it back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155878