[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked 17 inline comments as done.
shafik added a comment.

@martong I don't follow the discussion on `VisitParmVarDecl` an what seems like an alternate fix. Can you elaborate?



================
Comment at: lib/AST/ASTImporter.cpp:3042
 
+  if (FoundByLookup) {
+    if (auto *MD = dyn_cast<CXXMethodDecl>(FoundByLookup)) {
----------------
martong wrote:
> It is not trivial why we add this block to the code, so I think a comment would be really appreciated here.
> I was thinking about something like this:
> ```
> +  // We do not allow more than one in-class prototype of a function.  This is
> +  // because AST clients like VTableBuilder asserts on this.  VTableBuilder
> +  // assumes that only one function can override a function. Building a redecl
> +  // chain would result there are more than one function which override the
> +  // base function (even if they are part of the same redecl chain inside the
> +  // derived class.)
> ```
Just for clarification, from a C++ perspective, I would say "in-class declaration" rather than "in-class prototype" and therefore `VTableBuilder` assumes only one in class declaration and building a redecal chain would result in more than one in-class declaration being present. 

Does that makes sense?


================
Comment at: unittests/AST/ASTImporterTest.cpp:2344
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDefTwoTUs) {
+  auto CodeTU0 =
----------------
balazske wrote:
> The previous tests use two TUs too so the name for this is not exact (here the code is different, maybe use `ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode`?). This test does roughly the same as `ImportOverriddenMethodTwiceDefinitionFirst` but with the definition last and out-of-class version. (Name can be `ImportOverriddenMethodTwiceOutOfClassDefLast` too.) I am not sure why is the `foo` used here (`B::F` can be imported instead).
I originally thought I did not need `foo` either but I could get it to import the definition of `B::f()` any other way.

I also saw similar behavior with `clang-import-test` where I need an expression that included something like `foo()` in order to obtain the out-of-line definition. It is not obvious to me why.

So in this case if I switch to using `BFP` instead of `FooDef` then this fails:

    auto *ToBFOutOfClass = FirstDeclMatcher<CXXMethodDecl>().match(
      ToTU, cxxMethodDecl(hasName("f"), isDefinition()));




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56936/new/

https://reviews.llvm.org/D56936