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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@ioeric, thanks for the review round!
Answering the most important comments, will shortly send changes to actually address the rest.



================
Comment at: clangd/ExpectedTypes.cpp:40
+
+llvm::Optional<std::string> encodeType(ASTContext &Ctx, QualType T) {
+  assert(!T.isNull());
----------------
ioeric wrote:
> 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.
This function's responsibility is to encode the type. There is code to strip the qualifiers from the types in `toEquivClass`.
The initial patch does not take qualifiers into account as none of the complicated conversion logic (qualifiers were taken into account there) the original patch had made much difference in the ranking measurements I made.
That said, this change does not aim to finalize the type encoding. I'll be looking into improving the type-based ranking after this lands, might re-add qualifiers if they turn out to be an improvement. Want to prove this with measurements, though.


================
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.
----------------
ioeric wrote:
> 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?
I want to leave it vague for now. Convertible means whatever we think is good for code completion ranking.
Formalizing means we'll either dig into the C++ encoding or be imprecise. 

Happy to add the docs, but they'll probably get outdated on every change. Reading the code is actually simpler to get what's going on at this point.


================
Comment at: clangd/ExpectedTypes.h:37
+  fromCompletionResult(ASTContext &Ctx, const CodeCompletionResult &R);
+  static llvm::Optional<OpaqueType> fromPreferredType(ASTContext &Ctx,
+                                                      QualType Type);
----------------
ioeric wrote:
> why "preferred type"? maybe add a comment?
That's the terminology that clang uses for completion's context type. Will add a comment, thanks!


================
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
----------------
ioeric wrote:
> What is the raw representation? A hash or the type name or USR?
A string representation of the usr, but users shouldn't rely on it.
The contract is: you can use it to compare for equality and nothing else, so the comment is actually accurate :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52273