sammccall added inline comments.
================ 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 { ---------------- ilya-biryukov wrote: > sammccall wrote: > > 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. > Would still want to keep it as a marker type just for the sake of indicating > what we return and documentation purposes. > It also adds some type safety (granted, not much) for some use-cases. > > There's still an option to go strings with `rawStr()` if needed. For documentation purposes, `using OpaqueType = std::string` or so seems like a reasonable compromise? This is very heavyweight for the amount of typesafety we get. (Apart from the class itself, you've got `==` and `!=`, we should definitely have `<<` as well, `DenseMapInfo<>` and `<` may get added down the line...) ================ Comment at: clangd/ExpectedTypes.h:15 +// +// When using clang APIs, we cannot determine if a type coming from an AST is +// convertible to another type without looking at both types in the same AST. ---------------- I think this largely rehashes the second sentence of the above para. I'd suggest this one focus more closely on what our model *is*: We define an encoding of AST types as opaque strings, which can be stored in the index. Similar types (such as `string` and `const string&`) are folded together, forming equivalence classes with the same encoding. ================ Comment at: clangd/ExpectedTypes.h:18 +// Unfortunately, we do not have ASTs for index-based completion, so we have use +// a stable encoding for the C++ types and map them into equivalence classes +// based on convertibility. ---------------- ("stable" might suggest across versions) ================ Comment at: clangd/ExpectedTypes.h:42 + /// completion context. + static llvm::Optional<OpaqueType> fromPreferredType(ASTContext &Ctx, + QualType Type); ---------------- I'd suggest just `fromType`, exposing this as the primary method, and then on `fromCompletionResult` document why it's different. Having the names suggest the underlying structure (that `fromType` is "more fundamental") aids understanding, and doesn't really feel like we're painting ourselves into a corner. Alternately, `fromCompletionContext` and `fromCompletionResult` would be more clearly symmetrical. ================ Comment at: clangd/ExpectedTypes.h:49 + /// rely on it. + llvm::StringRef rawStr() const { return Data; } + ---------------- nit: if you keep this class, call this raw() for consistency with symbolid?( ================ Comment at: clangd/ExpectedTypes.h:59 +private: + static llvm::Optional<OpaqueType> encode(ASTContext &Ctx, QualType Type); + explicit OpaqueType(std::string Data); ---------------- any reason to put this in the header? ================ Comment at: unittests/clangd/ExpectedTypeTest.cpp:29 + +class ASTTest : public ::testing::Test { +protected: ---------------- This seems fine as a fixture, but I'd merge with the subclass - tests should be easy to read! ================ Comment at: unittests/clangd/ExpectedTypeTest.cpp:51 + +class ConvertibleToMatcher + : public ::testing::MatcherInterface<const ValueDecl *> { ---------------- "convertible to" is a problematic description for a couple of reasons: - it's a relationship between types, but encapsulates unrelated semantics to do with completions - it's a higher level of abstraction than the code under test As discussed offline/below, I think the best remedy here is just to drop this matcher - it's only used in one test that can now live with something much simpler. ================ Comment at: unittests/clangd/ExpectedTypeTest.cpp:107 + std::map<std::string, EquivClass> + buildEquivClasses(llvm::ArrayRef<const ValueDecl *> Decls) { + std::map<std::string, EquivClass> Classes; ---------------- nit: any reason this takes Decl*s instead of strings? would be a bit terser not to wrap the args in decl() ================ Comment at: unittests/clangd/ExpectedTypeTest.cpp:110 + for (auto *D : Decls) { + auto Type = OpaqueType::fromCompletionResult( + ASTCtx(), CodeCompletionResult(D, CCP_Declaration)); ---------------- I think we could simplify by only testing the type encodings/equiv classes here, and relying on the function -> return type conversion happening elsewhere. ================ Comment at: unittests/clangd/ExpectedTypeTest.cpp:142 + decl("iptr"), decl("bptr"), decl("user_type")}; + EXPECT_THAT(buildEquivClasses(Decls), ClassesAre({{"b", "i", "ui", "ll"}, + {"f", "d"}, ---------------- Ooh, we should avoid folding bool with other integer types I think! You hardly ever want to pass a bool where an int is expected. (The reverse int -> bool is somewhat common, but no more than pointer -> bool... type equivalence isn't the right hammer to solve that case). ================ Comment at: unittests/clangd/ExpectedTypeTest.cpp:173 + +TEST_F(ExpectedTypeConversionTest, FunctionReturns) { + build(R"cpp( ---------------- I think this test is a bit too high-level - there are big abstractions between the test code and the code under test (which is pretty simple). I'd suggest just `EXPECT_EQ( OpaqueType::fromCompletionResult(ASTCtx(), decl("returns_int")), OpaqueType::fromExpectedType(ASTCtx(), decl("int_"));` (If you think there's something worth testing for the pointer case, I'd do that instead rather than as well) 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