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

[Ilya Biryukov via Phabricator via cfe-commits]

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