[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-07-01 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-07-01 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-07-01 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-07-01 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-07-01 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-30 Thread Aleksei Sidorin via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-25 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-25 Thread Balázs Kéri via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Gabor Marton via Phabricator via cfe-commits
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] =

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Balázs Kéri via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Balázs Kéri via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-24 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-23 Thread Aleksei Sidorin via Phabricator via cfe-commits
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 __

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-21 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-06-21 Thread Gabor Marton via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-05-26 Thread Aleksei Sidorin via Phabricator via cfe-commits
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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

2019-05-24 Thread Gabor Marton via Phabricator via cfe-commits
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