ilya-biryukov 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 {
----------------
sammccall wrote:
> 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...)
As discussed offline, kept the class with an expectation that we'll use the
fixed-size representation at some point. Added a comment that it can be viewed
as a strong typedef to string for now.
================
Comment at: clangd/ExpectedTypes.h:42
+ /// completion context.
+ static llvm::Optional<OpaqueType> fromPreferredType(ASTContext &Ctx,
+ QualType Type);
----------------
sammccall wrote:
> 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.
Done. Using `fromType` now.
================
Comment at: clangd/ExpectedTypes.h:59
+private:
+ static llvm::Optional<OpaqueType> encode(ASTContext &Ctx, QualType Type);
+ explicit OpaqueType(std::string Data);
----------------
sammccall wrote:
> any reason to put this in the header?
It uses a private constructor of the class, so it seems natural for it to be a
private static function.
================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:51
+
+class ConvertibleToMatcher
+ : public ::testing::MatcherInterface<const ValueDecl *> {
----------------
sammccall wrote:
> "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.
Done. It was needed only for one test, testing it diretly now.
================
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"},
----------------
sammccall wrote:
> 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).
Fair point, changed this. Bool requires a whole different handling anyway, e.g.
I definitely want my pointers to be boosted in if conditions.
================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:173
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+ build(R"cpp(
----------------
sammccall wrote:
> 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)
Done. There is still a helper variable per case (I think it improves the
readability a little), but otherwise the test is more straightforward now.
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
