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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
