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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Shafik,
I have realized what's the problem with the `ctu-main` test. When we import the body and set the new body to the existing FunctionDecl then the parameters are still the old parameters so the new body does not refer to the formal parameters! This way the analyzer legally thinks that the parameter is not used inside the function because there is no DeclRef to that :(
This could be solved only if we merge *every* parts precisely, including the parameters, body, noexcept specifier, etc. But this would be a huge work.

Also, the test in `ctu-main` contains ODR violations. E.g, below `fcl` is first just a protoype, then it is defined in-class in the second TU.

  // ctu-main.cpp
  class mycls {
  public:
    int fcl(int x);
    //...
  
  // ctu-other.cpp
   class mycls {
   public:
    int fcl(int x) {
      return x + 5;
    }
    //...

In the second TU it should be defined out-of-class.

So my suggestion is to

1. use out-of-class definition of functions in `ctu-other.cpp`. Since I tried it already i have the diff for the lit tests, attached.F7837319: lit_tests.patch <https://reviews.llvm.org/F7837319>
2. skip the case when there is a definition in the 'from' context and let it do the redecl chain.

For 2) I was thinking about this:

  if (FoundByLookup) {
    if (auto *MD = dyn_cast<CXXMethodDecl>(FoundByLookup)) {
      if (D->getLexicalDeclContext() == D->getDeclContext()) {
        if (!D->doesThisDeclarationHaveABody())
          return cast<Decl>(const_cast<FunctionDecl *>(FoundByLookup));
        else {
          // Let's continue and build up the redecl chain in this case.
          // FIXME Merge the functions into one decl.
        }
      }
    }
  }

Later, we must implement the case when we have to merge the definition into the prototype properly, but that should be definitely another patch. 
Actually, this case comes up mostly with class template specializations , because different specializations may have a function prototype in one TU, but a definition for that in another TU (see e.g. the test `MergeFieldDeclsOfClassTemplateSpecialization`).



================
Comment at: lib/AST/ASTImporter.cpp:3042
 
+  if (FoundByLookup) {
+    if (auto *MD = dyn_cast<CXXMethodDecl>(FoundByLookup)) {
----------------
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.)
```


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

https://reviews.llvm.org/D56936