nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:470
+ ExpectedHint{": S1", "x"},
+ ExpectedHint{": S2::Inner<int>", "y"});
}
----------------
mizvekov wrote:
> nridge wrote:
> > mizvekov wrote:
> > > mizvekov wrote:
> > > > nridge wrote:
> > > > > This test expectation change is suspicious. The type is being printed
> > > > > with `PrintingPolicy::SuppressScope=true`, shouldn't that still be
> > > > > respected?
> > > > Thanks for pointing that out, I will take a look, but I suspect this to
> > > > be some TypePrinter issue.
> > > Could you explain to me why the former behavior is more desirable here?
> > >
> > > I initially understood that this hint should provide the type written in
> > > a form that would actually work if it replaced the 'auto'.
> > > It is strange, but if it is just meant as a hint, it's still fine I guess.
> > >
> > > Or maybe this was broken in the first place and just was just missing a
> > > FIXME?
> > The type is being printed with a `PrintingPolicy` which has the
> > [non-default SuppressScope flag
> > set](https://searchfox.org/llvm/rev/e158b5634aa67ea3039a62c3d8bda79b77b3b21c/clang-tools-extra/clangd/InlayHints.cpp#34).
> >
> > The
> > [documentation](https://searchfox.org/llvm/rev/e158b5634aa67ea3039a62c3d8bda79b77b3b21c/clang/include/clang/AST/PrettyPrinter.h#129)
> > of this flag says "Suppresses printing of scope specifiers", which I
> > understand to mean both namespace and class scope specifiers.
> >
> > If you're asking why we are using this flag for inlay hints, it's to keep
> > the hints short so they don't take up too much horizontal space. Since the
> > hints are displayed inline, hints that are too long can result in the line
> > of code visually extending past the width of the editor window, and we try
> > to minimize the impact of this.
> I see.
>
> The problem is that the ElaboratedType sugar does not respect this when
> printing the nested name specifier it contains.
> The quick fix of trying to make it respect it shows that there are a bunch of
> tests that expect it not to.
> It seems that specific policy is used in places that need it to recover the
> type as written.
>
> So if we want this behavior, we probably need to extend the printing policy
> with a new flag here.
Thanks for the explanation!
I think this test expectation change is fine then, and would just suggest
adding a "// FIXME: SuppressScope is not respected in this case" comment
perhaps.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110216/new/
https://reviews.llvm.org/D110216
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
