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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/ExpectedTypes.cpp:27
+    return llvm::None;
+  auto *VD = llvm::dyn_cast<ValueDecl>(R.Declaration);
+  if (!VD)
----------------
maybe add a comment what `ValueDecl` covers roughly? E.g. functions, classes, variables etc.


================
Comment at: clangd/ExpectedTypes.cpp:40
+
+llvm::Optional<std::string> encodeType(ASTContext &Ctx, QualType T) {
+  assert(!T.isNull());
----------------
IIUC, we also encode the qualifiers into the final representation? If so, have you considered the underlying type without qualifiers? It seems to me this might be too restrictive for type-based boosting. For code completion ranking, I think type qualifiers (`const` etc) can be separate signals.


================
Comment at: clangd/ExpectedTypes.h:10
+// A simplified model of C++ types that can be used to check whether they are
+// convertible between each other without looking at the ASTs.
+// Used for code completion ranking.
----------------
We might want to formalize what "convertible" means here. E.g. does it cover conversion between base and derived class? Does it cover double <-> int conversion?


================
Comment at: clangd/ExpectedTypes.h:29
+namespace clangd {
+/// An opaque representation of a type that can be computed based on clang AST
+/// and compared for equality. The encoding is stable between different ASTs,
----------------
The name seems opaque ;) Why is it `opaque`? 


================
Comment at: clangd/ExpectedTypes.h:37
+  fromCompletionResult(ASTContext &Ctx, const CodeCompletionResult &R);
+  static llvm::Optional<OpaqueType> fromPreferredType(ASTContext &Ctx,
+                                                      QualType Type);
----------------
why "preferred type"? maybe add a comment?


================
Comment at: clangd/ExpectedTypes.h:40
+
+  /// Get the raw byte representation of the type. Bitwise equality of the raw
+  /// data is equivalent to equality operators of SType itself. The raw
----------------
What is the raw representation? A hash or the type name or USR?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52273