[clangd] Implement hover for string literals

[v1nh1shungry]

Summary

Show string-literals' type and size in a hover card

Issue related: https://github.com/clangd/clangd/issues/1016

[tom-anders]

clang-tools-extra/clangd/Hover.cpp

785

Is it really useful to show the contents inside the Hover? You already see the contents of the string literal anyway, so I don't think this adds any value

788

(nit)

814

Hmm so what's stopping us from adding support for C here?

820

I think you should prefer llvm::formatv here

[v1nh1shungry]

@tom-anders Thanks for reviewing and for your suggestions!

clang-tools-extra/clangd/Hover.cpp

785

I added this comment only because others show the contents. I think you're right.

814

Because I'm not that familiar with C's data types and standards.

[v1nh1shungry]

Improve the patch

[v1nh1shungry]

Support different C++ standards

[v1nh1shungry]

Improve the patch

[v1nh1shungry]

Seems like I have made things too complex :(

Should work for C, C++, and other languages now.

[v1nh1shungry]

Improve the patch

[kadircet]

clang-tools-extra/clangd/Hover.cpp

827

can you extract this into a function, similar to the cases below?

830

i think it's a good idea to also include the size here (e.g. L"XYZ", would be 8 bytes).

[v1nh1shungry]

Apply the reviewing suggestions

[v1nh1shungry]

@kadircet Thanks for your suggestions!

[v1nh1shungry]

Modify the name in the hover info

[kadircet]

thanks, lgtm!

[v1nh1shungry]

@kadircet Could you please help me commit it? Thanks a lot!

[kadircet]

sure i am happy to land, @tom-anders do you have anything else to add ?

[tom-anders]

In D137650#3919089, @kadircet wrote:

sure i am happy to land, @tom-anders do you have anything else to add ?

LGTM!

clang-tools-extra/clangd/Hover.cpp

if (const auto *D = QT->getAsTagDecl()) {
       HI.Documentation = getDeclComment(ASTCtx, *CommentD);
       enhanceFromIndex(HI, *CommentD, Index);
     }
   }
 
   return HI;
 }
 
+HoverInfo getStringLiteralContents(const StringLiteral *SL,
+                                   const PrintingPolicy &PP) {
+  HoverInfo HI;
+
+  HI.Name = "string-literal";
+  HI.Size = (SL->getLength() + 1) * SL->getCharByteWidth();
+  HI.Type = SL->getType().getAsString(PP).c_str();
+
+  return HI;
+}
+
 bool isLiteral(const Expr *E) {
   // Unfortunately there's no common base Literal classes inherits from
   // (apart from Expr), therefore these exclusions.
   return llvm::isa<CompoundLiteralExpr>(E) ||
          llvm::isa<CXXBoolLiteralExpr>(E) ||
          llvm::isa<CXXNullPtrLiteralExpr>(E) ||
          llvm::isa<FixedPointLiteral>(E) || llvm::isa<FloatingLiteral>(E) ||
          llvm::isa<ImaginaryLiteral>(E) || llvm::isa<IntegerLiteral>(E) ||
-         llvm::isa<StringLiteral>(E) || llvm::isa<UserDefinedLiteral>(E);
+         llvm::isa<UserDefinedLiteral>(E);
 }
 
 llvm::StringLiteral getNameForExpr(const Expr *E) {
   // FIXME: Come up with names for `special` expressions.
   //
   // It's an known issue for GCC5, https://godbolt.org/z/Z_tbgi. Work around
   // that by using explicit conversion constructor.
   //
   // TODO: Once GCC5 is fully retired and not the minimal requirement as stated
   // in `GettingStarted`, please remove the explicit conversion constructor.
   return llvm::StringLiteral("expression");
 }
 
 // Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
 llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST,
                                            const PrintingPolicy &PP,
                                            const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
     return llvm::None;
 
   HoverInfo HI;
+  // Print the type and the size for string literals
+  if (const StringLiteral *SL = dyn_cast<StringLiteral>(E))
+    return getStringLiteralContents(SL, PP);
   // For `this` expr we currently generate hover with pointee type.
   if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E))
     return getThisExprHoverContents(CTE, AST.getASTContext(), PP);
   if (const PredefinedExpr *PE = dyn_cast<PredefinedExpr>(E))
     return getPredefinedExprHoverContents(*PE, AST.getASTContext(), PP);
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
     HI.Type = printType(E->getType(), AST.getASTContext(), PP);
     HI.Value = *Val;
     HI.Name = std::string(getNameForExpr(E));
     return HI;

clang-tools-extra/clangd/unittests/HoverTests.cpp

llvm::StringRef Tests[] = {
           )cpp",
       // literals
       "auto x = t^rue;",
       "auto x = ^(int){42};",
       "auto x = ^42.;",
       "auto x = ^42.0i;",
       "auto x = ^42;",
       "auto x = ^nullptr;",
-      "auto x = ^\"asdf\";",
   };
 
   for (const auto &Test : Tests) {
     SCOPED_TRACE(Test);
 
     Annotations T(Test);
     TestTU TU = TestTU::withCode(T.code());
     TU.ExtraArgs.push_back("-std=c++17");
struct {
     const std::function<void(HoverInfo &)> ExpectedBuilder;
   } Cases[] = {
       {"auto x = [['^A']]; // character literal",
        [](HoverInfo &HI) {
          HI.Name = "expression";
          HI.Type = "char";
          HI.Value = "65 (0x41)";
        }},
+      {"auto s = ^[[\"Hello, world!\"]]; // string literal",
+       [](HoverInfo &HI) {
+         HI.Name = "string-literal";
+         HI.Size = 14;
+         HI.Type = "const char[14]";
+       }},
       {
           R"cpp(// Local variable
             int main() {
               int bonjour;
               ^[[bonjour]] = 2;
               int test1 = bonjour;
             }
           )cpp",