[PATCH] D52273: [clangd] Initial implementation of expected types

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ExpectedTypes.cpp:12
+
+static llvm::Optional<QualType> toEquivClass(ASTContext &Ctx, QualType T) {
+  if (T.isNull() || T->isDependentType())
----------------
returning QualType vs Type*? It seems we strip all qualifiers, seems clearest for the return type to reflect that.


================
Comment at: clangd/ExpectedTypes.cpp:15
+    return llvm::None;
+  T = T.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+  if (T->isIntegerType() && !T->isEnumeralType())
----------------
Maybe we want Ctx.getUnqualifiedArrayType here or (more likely?) do array-to-pointer decay?


================
Comment at: clangd/ExpectedTypes.cpp:16
+  T = T.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+  if (T->isIntegerType() && !T->isEnumeralType())
+    return QualType(Ctx.IntTy);
----------------
wow, "enumeral" might be my favorite c++-made-up word, displacing "emplace"...


================
Comment at: clangd/ExpectedTypes.cpp:25
+typeOfCompletion(const CodeCompletionResult &R) {
+  if (!R.Declaration)
+    return llvm::None;
----------------
nit: dyn_cast_or_null below instead?


================
Comment at: clangd/ExpectedTypes.cpp:30
+    return llvm::None;
+  QualType T = VD->getType().getCanonicalType().getNonReferenceType();
+  if (!T->isFunctionType())
----------------
nit: is canonicalization necessary here? you do it in toEquivClass
(I guess dropping references is, for the function type check)


================
Comment at: clangd/ExpectedTypes.cpp:33
+    return T;
+  // Functions are a special case. They are completed as 'foo()' and we want to
+  // match their return type rather than the function type itself.
----------------
nit: I'd put the special case in the if() block, but up to you


================
Comment at: clangd/ExpectedTypes.cpp:37
+  // after the function name, e.g. `std::cout << std::endl`.
+  return T->getAs<FunctionType>()->getReturnType().getNonReferenceType();
+}
----------------
dropping references seems redundant here, as you do it again later


================
Comment at: clangd/ExpectedTypes.cpp:46
+  llvm::SmallString<128> Out;
+  if (index::generateUSRForType(T, Ctx, Out))
+    return llvm::None;
----------------
I think ultimately we may want to replace this with a custom walker:
 - we may want to ignore attributes (e.g. const) or bail out in some cases
 - generateUSRForType may not have the exact semantics we want for other random reasons
 - we can do tricks with hash_combine to avoid actually building huge strings we don't care about

not something for this patch, but maybe a FIXME?



================
Comment at: clangd/ExpectedTypes.cpp:71
+    return llvm::None;
+  T = toEquivClass(Ctx, *T);
+  if (!T)
----------------
can you reuse fromPreferredType for the rest?


================
Comment at: clangd/ExpectedTypes.h:32
+/// this allows the representation to be stored in the index and compared with
+/// types coming from a different AST later.
+class OpaqueType {
----------------
Does this need to be a separate class rather than using `std::string`?
There are echoes of `SymbolID` here, but there were some factors that don't apply here:
 - it was fixed-width
 - memory layout was important as we stored lots of these in memory
 - we hashed them a lot and wanted a specific hash function

I suspect at least initially producing a somewhat readable std::string a la USRGeneration would be enough.


================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:79
+         << (Matched ? "matched" : "did not match")
+         << "\n\tTarget type: " << To.getAsString()
+         << "\n\tSource value type: " << V->getType().getAsString();
----------------
note that if you think it's useful you can To.dump(*L->stream())
Maybe this is more interesting if/when we have a custom visitor.


================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:92
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+  build(R"cpp(
----------------
I really like the declarative equivalence-class setup of the tests.

A couple of suggestions:
 - maybe store the equivalence classes as groups of strings rather than decls, and lazily grab the decls. It's easier to tersely represent them...
 - I think the "convertibleTo" DSL obscures/abstracts the actual APIs you're testing - they build opaque types, and you're asserting equality.
 - pairwise assertion messages may not give enough context: if you expect a == b == c, and a != b, then whether a == c and b == c are probably relevant

I'd consider actually building up the equivalence classes `map<OpaqueType, set</*decl*/string>>` and writing a `MATCHER_P2(ClassesAre, /*vector<set<string>>*/Classes, /*ParsedAST*/AST, "classes are " + testing::PrintToString(Classes))`

That way the actual and expected equivalence classes will be dumped on failure, and you can still grab the decls/types from the AST to dump their details.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52273