[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction
Nathan Ridge via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 27 17:49:55 PDT 2021
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
More information about the cfe-commits
mailing list