mizvekov added inline comments.

================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:470
+                  ExpectedHint{": S1", "x"},
+                  ExpectedHint{": S2::Inner<int>", "y"});
 }
----------------
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.


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

Reply via email to