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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
