This revision was automatically updated to reflect the committed changes.
Closed by commit rC352436: [ASTImporter] Fix handling of overriden methods
during ASTImport (authored by shafik, committed by ).
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56936/new/
http
teemperor accepted this revision.
teemperor added a comment.
LGTM, thanks!
Comment at: unittests/AST/ASTImporterTest.cpp:2271
+struct D:B { void f(){}; };
+ )";
+ auto BFP =
I think this should be formatted in the same way as the snippet above.
shafik updated this revision to Diff 183923.
shafik marked 2 inline comments as done.
shafik added a comment.
Addressing comments on commenting and naming.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56936/new/
https://reviews.llvm.org/D56936
Files:
lib/AST/ASTImporter.cpp
test/A
balazske added inline comments.
Comment at: unittests/AST/ASTImporterTest.cpp:2312
+ cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+ auto BFPIsDefP = cxxMethodDecl(
+ hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Shafik, thanks for addressing the comments. This looks good to me now!
Comment at: lib/AST/ASTImporter.cpp:3042
+ if (FoundByLookup) {
+if (auto *MD = dyn_cast(Found
shafik updated this revision to Diff 183406.
shafik marked 3 inline comments as done.
shafik added a comment.
Changes based on comments
- Refactoring ImportFunctionDeclBody to return Error
- Refactoring test/Analysis/ctu-main.cpp based on Gabor's patch
- Removing merging of function body for late
shafik added a comment.
@martong so I just tested your `lit-tests.patch` and it looks good!
Comment at: lib/AST/ASTImporter.cpp:2959
+}
+
ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
balazske wrote:
> Code formatting is not OK in this fun
balazske added inline comments.
Comment at: lib/AST/ASTImporter.cpp:2950
+ExpectedStmt ASTNodeImporter::ImportFunctionDeclBody( FunctionDecl *FromFD,
FunctionDecl *ToFD ) {
+if (Stmt *FromBody = FromFD->getBody()) {
It would be better to return `Error` ins
shafik updated this revision to Diff 183251.
shafik added a comment.
Addressing comments on naming in the unit test and refactoring of duplicated
code.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56936/new/
https://reviews.llvm.org/D56936
Files:
lib/AST/ASTImporter.cpp
unittests
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_cas
martong added a comment.
> So the problem is that there are references to ParmVarDecl from inside
> function body and at import of ParmVarDecl always a new one is created even
> if there is an already existing (in the existing function prototype)?
Yes. During the import of the body we import a
balazske added a comment.
So the problem is that there are references to `ParmVarDecl` from inside
function body and at import of `ParmVarDecl` always a new one is created even
if there is an already existing (in the existing function prototype)? Maybe it
works in `VisitParmVarDecl` to search f
martong added inline comments.
Comment at: lib/AST/ASTImporter.cpp:3046
+if (!D->doesThisDeclarationHaveABody())
+ return cast(const_cast(FoundByLookup));
+else {
balazske wrote:
> The `cast` should not be needed here (and is not done at
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
martong added a comment.
I have found some other minor things in the tests.
Comment at: unittests/AST/ASTImporterTest.cpp:2275
+ auto DFDef = cxxMethodDecl(
+ hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+ auto FDefAll = cxxMethodDecl(hasName("f
martong added a comment.
> @martong the only issue is that I am seeing a regression on
> Analysis/ctu-main.cpp when I run check-clang. I am going to look into it but
> if you have any insights that would be helpful.
I am looking into it and will come back with what I have found.
CHANGES SINCE
martong added a comment.
Hi Shafik, thank you for this patch! Generally it looks quite okay to me, but I
have a few minor comments.
Comment at: lib/AST/ASTImporter.cpp:3049
+ // Import the body of D and attach that to FoundByLookup.
+ // This should probably
balazske added inline comments.
Comment at: lib/AST/ASTImporter.cpp:3046
+if (!D->doesThisDeclarationHaveABody())
+ return cast(const_cast(FoundByLookup));
+else {
The `cast` should not be needed here (and is not done at the other return
shafik added a comment.
@martong the only issue is that I am seeing a regression on
`Analysis/ctu-main.cpp` when I run `check-clang`. I am going to look into it
but if you have any insights that would be helpful.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56936/new/
https://reviews
shafik created this revision.
shafik added reviewers: martong, teemperor, balazske, aaron.ballman.
Herald added a subscriber: rnkovacs.
When importing classes we may add a CXXMethodDecl more than once to a
CXXRecordDecl. This patch will fix the cases we currently know about and handle
both the c
20 matches
Mail list logo
