martong added a comment.
Jenkins is green:
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29809/
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62375/new/
https://reviews.llvm.org/D62375
___
cfe-commits mail
martong added a comment.
@shafik I've been looking for any lldb regression in our Mac machine, could not
find any. Now I am looking at
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ . I don't expect
regression because here we changed logic about the error handling only, so I'd
expec
This revision was automatically updated to reflect the committed changes.
Closed by commit rL364771: [ASTImporter] Mark erroneous nodes in from ctx
(authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://revie
martong marked 3 inline comments as done.
martong added a comment.
> Thank you for the explanation. I got the idea of this patch anyway, but it
> will be definitely useful for anyone digging into the code. Should we make it
> a comment for ImportPathTy?
Ok, I have added the explanation to `Impo
martong updated this revision to Diff 207288.
martong marked an inline comment as done.
martong added a comment.
- Move ImportPathTy's funcitons in-class, add class comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62375/new/
https://reviews.ll
a_sidorin added a comment.
Hello Gabor,
Thank you for the explanation. I got the idea of this patch anyway, but it will
be definitely useful for anyone digging into the code. Should we make it a
comment for ImportPathTy?
Comment at: clang/lib/AST/ASTImporter.cpp:8682
+
+void
martong marked 2 inline comments as done.
martong added inline comments.
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5000
EXPECT_TRUE(ImportedOK);
}
balazske wrote:
> I see now that this test is really not required, the previous test checks the
> a
balazske added inline comments.
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5000
EXPECT_TRUE(ImportedOK);
}
I see now that this test is really not required, the previous test checks the
almost same thing in case of `class B` and `NS` (but more test
martong added inline comments.
Comment at: clang/lib/AST/ASTImporter.cpp:8668
+
+bool ASTImporter::ImportPathTy::hasCycleAtBack() {
+ return Aux[Nodes.back()] > 1;
a_sidorin wrote:
> const?
Thanks! I made to be const.
Comment at: clang/lib/AST
martong marked an inline comment as done.
martong added inline comments.
Comment at: clang/lib/AST/ASTImporter.cpp:7892
-// Error encountered for the first time.
-assert(!getImportDeclErrorIfAny(FromD) &&
We may set up an error multiple times now, but t
martong updated this revision to Diff 206254.
martong marked 5 inline comments as done.
martong added a comment.
- Use make_scope_exit
- Make hasCycleAtBack const
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62375/new/
https://reviews.llvm.org/D62
martong marked 2 inline comments as done.
martong added inline comments.
Comment at: clang/lib/AST/ASTImporter.cpp:8647
- assert(ImportDeclErrors.find(From) == ImportDeclErrors.end() &&
- "Setting import error allowed only once for a Decl.");
ImportDeclErrors[From] =
martong updated this revision to Diff 206244.
martong added a comment.
- Add back an assertion in setImportDeclError(), remove the condition in
Import()
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62375/new/
https://reviews.llvm.org/D62375
File
balazske added inline comments.
Comment at: clang/lib/AST/ASTImporter.cpp:8647
- assert(ImportDeclErrors.find(From) == ImportDeclErrors.end() &&
- "Setting import error allowed only once for a Decl.");
ImportDeclErrors[From] = Error;
We should not rem
martong updated this revision to Diff 206228.
martong added a comment.
- Remove the macro and use std::string.op+
- We may set up an error twice in case of a cycle, thus remove the assert
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62375/new/
htt
balazske added inline comments.
Comment at: clang/lib/AST/ASTImporter.cpp:7840
+ // Push FromD to the stack, and remove that when we return.
+ ImportPathBuilder PathRAII(ImportPath, FromD);
It is possible to use the `make_scope_exit` instead, this results in
martong added a comment.
Alexei,
I think I still owe you some explanation about this patch.
I do consider this patch as one of the most intricate patches regarding
ASTImporter.
I'd like to answer the following questions in this comment:
What is an ImportPath and why do we need to track it?
What
a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.
Thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62375/new/
https://reviews.llvm.org/D62375
__
martong added a comment.
Alexei,
Thank you for the review, I have added a test which demonstrates the changes.
By tracking the import paths and cycles we implement a very strict error
handling mechanism, but this seems to be the way to avoid reaching any faulty
AST nodes for the ASTImporter cli
martong updated this revision to Diff 206022.
martong added a comment.
- Add test ErrorPropagatesThroughImportCycles
- Change existing test to new behaviour
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62375/new/
https://reviews.llvm.org/D62375
F
a_sidorin added a comment.
Hi Gabor,
Could you please provide any test for the import itself?
Comment at: clang/lib/AST/ASTImporter.cpp:8668
+
+bool ASTImporter::ImportPathTy::hasCycleAtBack() {
+ return Aux[Nodes.back()] > 1;
const?
Repository:
rG LLVM Gi
martong created this revision.
martong added a reviewer: a_sidorin.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: clang.
During import of a specific Decl D, it may happen t
22 matches
Mail list logo
