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