sammccall marked an inline comment as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/AST.cpp:491
+ // We'll examine visible specializations and see if they yield a unique type.
+ bool VisitParmVarDecl(ParmVarDecl *PVD) {
+ if (!PVD->getType()->isDependentType())
----------------
Trass3r wrote:
> Since this is implemented generically in the AST, can this functionality
> easily be reused for a type inlay hint?
In principle yes. Rather than expand the scope further, I'll send a followup
patch.
================
Comment at: clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp:88
+ StartsWith("fail: Could not deduce"));
+ EXPECT_EQ(apply("auto X = [](^auto){return 0;}; int Y = X(42);"),
+ "auto X = [](int){return 0;}; int Y = X(42);");
----------------
nridge wrote:
> sammccall wrote:
> > nridge wrote:
> > > Maybe add a conflicting-deduction case? (I know the GetDeducedType test
> > > already has one, but here it would be especially wrong.)
> > >
> > > Also, should we perhaps disable this in header files (maybe excepting
> > > function-local symbols), since an including source file could have
> > > additional instantiations?
> > > Maybe add a conflicting-deduction case? (I know the GetDeducedType test
> > > already has one, but here it would be especially wrong.)
> > Done.
> >
> > > Also, should we perhaps disable this in header files (maybe excepting
> > > function-local symbols), since an including source file could have
> > > additional instantiations?
> >
> > Disabling features in headers seems sad. Detecting when the function is
> > local doesn't seem like a great solution to this:
> > - it's going to be hard to detect the common ways functions are hidden
> > within headers (`detail` namespaces, inaccessible members, ...)
> > - it's still going to be wrong when some instantiations come via (local)
> > templates that are themselves instantiated from the including file
> >
> > I suspect in most cases where multiple instantiations are intended we're
> > going to see either no instantiations or many. Can we try it and see if
> > there are false positive complaints?
> To clarify, I meant excepting only cases like this:
>
> ```
> // header.hpp
> void foo(std::function<void(ConcreteType)> callback);
> void bar() {
> // We know this will only be instantiated with ConceteType,
> // regardless of what an including source file does
> foo([](auto x){});
> }
> ```
>
> but I'm sympathetic to the argument that neither false-positives (we offer a
> refactoring that breaks code) nor false-negative (we fail to offer a
> refactoring the user wants to make) are super impactful here and so we should
> do the easier thing (i.e. not try to restrict at all).
OK, will leave the checks out and see how it goes.
(I agree that's probably the most important case, but I haven't worked in a
c++20 codebase so I don't know how abbreviated function templates will play out)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119537/new/
https://reviews.llvm.org/D119537
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
