kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks!
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1250
const SourceManager &SM = AST.getSourceManager();
- const auto &ConvertedMainFileIncludes =
- convertIncludes(SM, AST.getIncludeStructure().MainFileIncludes);
- const auto &HoveredInclude = convertIncludes(SM, llvm::ArrayRef{Inc});
+ const auto &Converted = convertIncludes(AST);
llvm::DenseSet<include_cleaner::Symbol> UsedSymbols;
----------------
sammccall wrote:
> kadircet wrote:
> > oops, looks like we were getting away with some dangling references.
> > the reference here is wrong, `convertIncludes` returns a value type. can
> > you fix that while here?
> this isn't dangling, it's lifetime extension.
>
> Can change it for style if you like, though?
you're right. i think it would be better to have it as value though.
================
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();
+ });
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155878/new/
https://reviews.llvm.org/D155878
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
