sammccall added a comment.
This looks pretty good in terms of behavior, some impl comments but we should
land this.
My main remaining concern is just that this will trigger too often when there's
no confusion over types, and clutter the display.
Some common cases I'm worried about:
- std::string -> basic_string<char>. Maybe we should special-case this to hide
the aka?
- `int64_t` vs `long long` et al. I have no idea what to do about this.
@kadircet hopefully has thoughts on what hover should look like
================
Comment at: clang-tools-extra/clangd/Hover.cpp:142
-std::string printType(QualType QT, const PrintingPolicy &PP) {
+HoverInfo::PrintedType printType(QualType QT, ASTContext &Context,
+ const PrintingPolicy &PP) {
----------------
nit: ASTCtx or Ctx to distinguish from clangd::Context
================
Comment at: clang-tools-extra/clangd/Hover.cpp:149
QT = QT->getAs<DecltypeType>()->getUnderlyingType();
std::string Result;
llvm::raw_string_ostream OS(Result);
----------------
nit: declare a `HoverInfo::PrintedType Result` and write directly into it
instead of these partial variables?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:160
OS.flush();
- QT.print(OS, PP);
- return Result;
+ std::string Type = Result + QT.getAsString(PP);
+ llvm::Optional<std::string> AKA;
----------------
prefer printing into the same string rather than copying into a new one
================
Comment at: clang-tools-extra/clangd/Hover.cpp:166
+ if (ShouldAKA)
+ AKA = Result + DesugaredTy.getAsString(PP);
+ }
----------------
The e.g. "class " prefix isn't needed in the a.k.a, so no need to copy strings
for that.
(and in fact I'm not sure it's possible to hit an AKA when this string is
nonempty)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:189
-std::string printType(const TemplateTemplateParmDecl *TTP,
- const PrintingPolicy &PP) {
- std::string Res;
- llvm::raw_string_ostream OS(Res);
- OS << "template <";
- llvm::StringRef Sep = "";
+HoverInfo::PrintedType printType(const TemplateTemplateParmDecl *TTP,
+ const PrintingPolicy &PP) {
----------------
How strongly do you feel about handling this case?
This code is much more surprising than the previous version and maybe we can
find a simpler way to express it, but all of this only matters if we have a
template-template-paramater with a non-type-template-parameter with nontrivial
sugar which obscures meaning.
That seems pretty unlikely to ever come up, and if it does I'm not sure
repeating the whole TTP is a great experience.
I'd suggest just dropping the possibility of an AKA type here until someone
runs into a case where it's a real issue...
================
Comment at: clang-tools-extra/clangd/Hover.cpp:689
+ auto PType = printType(PrettyThisType, ASTCtx, PP);
+ HI.Definition = llvm::formatv("{0}", PType.Type);
+ if (PType.AKA)
----------------
HI.Definition = std::move(PType.Type)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:689
+ auto PType = printType(PrettyThisType, ASTCtx, PP);
+ HI.Definition = llvm::formatv("{0}", PType.Type);
+ if (PType.AKA)
----------------
sammccall wrote:
> HI.Definition = std::move(PType.Type)
this formatting of the definition appears twice, pull out a function? `string
typeAsDefinition(PrintedType)` or so
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1093
// - `int param2 = 5`
- Output.addParagraph().appendText("→ ").appendCode(*ReturnType);
+ std::string Buffer;
+ llvm::raw_string_ostream OS(Buffer);
----------------
appendCode(llvm::to_string(ReturnType))
(from ScopedPrinter.h)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1271
OS << " = " << *P.Default;
+ if (P.Type && P.Type->AKA)
+ OS << llvm::formatv(" (aka {0})", *P.Type->AKA);
----------------
Hmm, this seems to render as:
```std::string Foo = "" (aka basic_string<char>)```
it's not *completely* clear what the aka refers to.
I don't have a better solution, though.
@kadircet any opinions here?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:166
+ if (ShouldAKA)
+ AKA = Result + DesugaredTy.getAsString(PP);
+ }
----------------
lh123 wrote:
> lh123 wrote:
> > lh123 wrote:
> > > It seems we lost `namespace qualifiers`.
> > > It always display `std::string` as `basic_string<char>`
> > Should we set `FullyQualifiedName` printing policy for `DesugaredTy`.
> Sometimes we need to distinguish certain types with the same name based on
> the `fully qualified name`.
> eg.
> ```
> namespace custom {
> template <typename T> struct vector {};
> } // namespace custom
>
> void code() {
> custom::vector<size_t> a;
> std::vector<size_t> b;
> }
> ```
> Currently, both `a` and `b` show `vector<unsigned long>`.
I'd lean against setting FullyQualifiedName for the AKA type. Clang doesn't for
diagnostics (I see `'std::string' aka 'basic_string<char>'`).
It is more specific and adds verbosity. It should be very rare that it is the
only way to find things out, e.g. in your example the *original* types would be
different as they include the namespace as spelled in the code.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:48
HI.Definition = "void foo()";
- HI.ReturnType = "void";
- HI.Type = "void ()";
+ HI.ReturnType = {"void", llvm::None};
+ HI.Type = {"void ()", llvm::None};
----------------
you could reduce the noise here by giving PrintedType a constructor from char*
or so...
================
Comment at: clang/include/clang/AST/ASTDiagnostic.h:38
+ /// whenever we remove significant sugar from the type.
+ QualType Desugar(ASTContext &Context, QualType QT, bool &ShouldAKA);
} // end namespace clang
----------------
when exposing this, we should probably give it a more specific name like
`desugarForDiagnostic`, so it's not confused with the various QT->desugar() or
getCanonicalType() type functions.
While renaming, it should start with a lowercase d.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114522/new/
https://reviews.llvm.org/D114522
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
