[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory

a.sidorin added inline comments. Comment at: unittests/AST/ASTImporterTest.cpp:32 + +static RunOptions getRunOptionsForLanguage(Language Lang) { + ArgVector BasicArgs; xazax.hun wrote: > I wonder if in the future it would be worth to use something else, like >

[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory

This revision was automatically updated to reflect the committed changes. Closed by commit rL321285: [ASTImporterTest] Add mandatory testing with '-fdelayed-template-parsing' (authored by a.sidorin, committed by ). Changed prior to commit: https://reviews.llvm.org/D41444?vs=127748&id=127903#toc

[PATCH] D41538: [analyzer] Fix some checker's output plist not containing the checker name #2

a.sidorin added a comment. Hello Gabor, For me, this patch looks much better than previous. I have some questions inline. Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:40 public: - BugType(class CheckName check, StringRef name, StringRef cat) - :

[PATCH] D41538: [analyzer] Fix some checker's output plist not containing the checker name #2

a.sidorin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:277 + BT_leakedvalist.reset(new BugType( + CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated], + "Leaked va_list", categories::MemoryError)); -

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

This revision was automatically updated to reflect the committed changes. Closed by commit rC322079: [ASTImporter] Fix missing SourceLoc import for ObjCMethodDecl selectors (authored by a.sidorin, committed by ). Repository: rC Clang https://reviews.llvm.org/D6550 Files: lib/AST/ASTImporter

[PATCH] D38694: [ASTImporter] Support importing CXXUnresolvedConstructExpr and UnresolvedLookupExpr

This revision was automatically updated to reflect the committed changes. Closed by commit rC322091: [ASTImporter] Support importing CXXUnresolvedConstructExpr and… (authored by a.sidorin, committed by ). Changed prior to commit: https://reviews.llvm.org/D38694?vs=127283&id=129095#toc Reposito

[PATCH] D41797: [analyzer] Suppress escape of this-pointer during construction.

a.sidorin added a comment. Hi Artem, I think that global suppression is fine. If one really wants to check such escapes, he can implement a separate callback for this. Repository: rC Clang https://reviews.llvm.org/D41797 ___ cfe-commits mailing

[PATCH] D41800: [analyzer] Use a custom program point for the check::NewAllocator callback.

a.sidorin accepted this revision. a.sidorin added inline comments. Comment at: test/Analysis/NewDelete-path-notes.cpp:44 // CHECK-NEXT: -// CHECK-NEXT:line6 +// CHECK-NEXT:line7 // CHECK-NEXT:col3 Not even a minor

[PATCH] D41800: [analyzer] Use a custom program point for the check::NewAllocator callback.

a.sidorin added inline comments. Comment at: test/Analysis/NewDelete-path-notes.cpp:44 // CHECK-NEXT: -// CHECK-NEXT:line6 +// CHECK-NEXT:line7 // CHECK-NEXT:col3 NoQ wrote: > a.sidorin wrote: > > Not even a minor

[PATCH] D43798: [analyzer] UndefinedAssignment: Fix warning message on implicit copy/move constructors.

a.sidorin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp:60 // Generate a report for this bug. + std::string Str; + llvm::raw_string_ostream OS(Str); SmallString<128>? Comment at: lib/StaticAn

[PATCH] D43714: [analyzer] Don't do anything when trivial-copying an empty class object.

a.sidorin accepted this revision. a.sidorin added a comment. Looks good! https://reviews.llvm.org/D43714 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

a.sidorin reopened this revision. a.sidorin added a comment. The changes were reverted: http://llvm.org/viewvc/llvm-project?rev=326432&view=rev Gabor, could you take a look? Repository: rC Clang https://reviews.llvm.org/D30691 ___ cfe-commits ma

[PATCH] D44079: [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls

a.sidorin created this revision. a.sidorin added reviewers: r.stahl, xazax.hun, jingham, szepet. Herald added subscribers: martong, rnkovacs. This patch introduces the ability to test an arbitrary sequence of imports between a given set of virtual source files. This should finally allow us t

[PATCH] D39159: [analyzer] Improves the logic of GenericTaintChecker identifying stdin.

a.sidorin accepted this revision. a.sidorin added a comment. LGTM. Repository: rC Clang https://reviews.llvm.org/D39159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43967: [ASTImporter] Add test helper Fixture

a.sidorin added a comment. Hello Gabor, Unfortunately, we did same or similar work: https://reviews.llvm.org/D44079. We have to decide what approach is going to be used. It doesn't mean that we should select only one - probably, we can use both approaches together and refactor or change our pa

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a.sidorin created this revision. a.sidorin added reviewers: xazax.hun, szepet, jingham. Herald added subscribers: martong, rnkovacs. - There are multiple reasons why field structures can be imported in wrong way. The simplest is the ability of field initializers and method bodies to refer fields

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a.sidorin added a comment. Hi Gabor, > Just a minor high level note. If https://reviews.llvm.org/D32947 will be > accepted once, we will need to reorder friends as well. Or alternatively, we > have to omit the check of friends in structural equivalence in > https://reviews.llvm.org/D32947. I'

[PATCH] D44154: [checker] PoC : Unsequenced Modification Checker

a.sidorin added a comment. Hi Paul, You didn't add any reviewer. Do you need a review for this checker? Are you going to contribute this code to the upstream? Repository: rC Clang https://reviews.llvm.org/D44154 ___ cfe-commits mailing list cfe

[PATCH] D43967: [ASTImporter] Add test helper Fixture

a.sidorin added a comment. Hello Gabor, Sorry for the delay with review: the patch is pretty big and it was hard to find time for reviewing it. I like the patch but it requires some cleanup like formatting changes. Could you please clang-format new code? Also, I'd like to avoid code duplication

[PATCH] D47532: [ASTImporter] Import the whole redecl chain of functions

a.sidorin added a comment. Hello Gabor, This patch is really cool. Unfortunately, right now I don't have enough time for reviewing large patches in a single review. Are you OK with incremental review? Comment at: lib/AST/ASTImporter.cpp:75 + template + llvm::SmallVector +

[PATCH] D47698: [ASTImporter] import macro source locations

a.sidorin added a comment. Hi Rafael, I apologize for the delay in review and hope to get ASTImporter patches reviewed on this weekend. Comment at: lib/AST/ASTImporter.cpp:7054 + // Map the FileID for to the "to" source manager. FileID ToID; 'for to' =>

[PATCH] D47698: [ASTImporter] import macro source locations

a.sidorin added a comment. Hi Rafael, I think the patch is great. But, honestly, I have never dealt with SourceLocation machinery closely, so some things are a bit unclear to me. Comment at: lib/AST/ASTImporter.cpp:7058 +const SrcMgr::ExpansionInfo &FromEx = FromSLoc.getE

[PATCH] D47450: [ASTImporter] Use InjectedClassNameType at import of templated record.

a_sidorin added a comment. Hello Balázs, Please clang-format the tests and delete injected-class-name-decl-1. Don't see any other issues. Comment at: lib/AST/ASTImporter.cpp:2132 +if (!DCXX->isInjectedClassName()) { + // In a record describing a template the

[PATCH] D47534: [ASTImporter] Add new tests about templated-described swing

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, Tests are always welcome. This patch is OK and can be committed after fixing nits without additional approval. Comment at: unittests/AST/ASTImporterTest.cpp

[PATCH] D47532: [ASTImporter] Import the whole redecl chain of functions

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, The patch LG, thank you for addressing my questions. Just some stylish nits. Comment at: unittests/AST/ASTImporterTest.cpp:2021 + +TEST_P(ImportFriendFunctio

[PATCH] D47367: [ASTImporter] Add ms compatibility to tests

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thank you! Could you describe the refactoring in the commit message? Comment at: unittests/AST/ASTImporterTest.cpp:212 + /// The verification is done by running

[PATCH] D47459: [ASTImporter] Eliminated some unittest warnings.

a_sidorin added a comment. Hello Balázs, The patch is mostly LG now, thank you! Comment at: unittests/AST/ASTImporterTest.cpp:495 " struct s { int x; long y; unsigned z; }; " - " (struct s){ 42, 0L, 1U }; }", + " (void) (struct s){ 42,

[PATCH] D51633: [ASTImporter] Added error handling for AST import.

a_sidorin added a comment. Hi Balázs, Finally, I have completed the review. Despite the fact that this patch is huge, I found only a few issues. Comment at: lib/AST/ASTImporter.cpp:4604 + if (Error Err = ImportDeclContext(D)) +// FIXME: Really ignore the error? +cons

[PATCH] D51633: [ASTImporter] Added error handling for AST import.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Balasz, I cannot find any problems with the latest changes. I suggest you to commit the patch to make further reviews easier. Comment at: lib/AST/ASTImporter.cpp:81

[PATCH] D45416: [AST, analyzer] Transform rvalue cast outputs to lvalues (fheinous-gnu-extensions)

This revision was automatically updated to reflect the committed changes. Closed by commit rL344864: [AST, analyzer] Transform rvalue cast outputs to lvalues (fheinous-gnu… (authored by a.sidorin, committed by ). Herald added subscribers: llvm-commits, dkrupp, donat.nagy. Changed prior to commit:

[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type

a_sidorin added a comment. Hi Gabor, Thank you for the patch. I looks reasonable but I have some questions. Comment at: lib/AST/ASTImporter.cpp:2310 +D->getUnderlyingType(), FoundTypedef->getUnderlyingType())) { + QualType FromUT = D->getUnderlyingType(

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Hi Gabor, I have tried to rebase it last week but there were some test failures. I hope to fix them this weekend and finally commit the patch. Repository: rC Clang https://reviews.llvm.org/D44100 ___ cfe-commits mailin

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

a_sidorin added a comment. Hi Gabor, I wonder if it is possible to get into situation where non-equivalent decls are marked equivalent with this patch? If yes, we can create a mapping between decls being imported and original decls as an alternative solution. However, I cannot find any counte

[PATCH] D53704: [ASTImporter] Import overrides before importing the rest of the chain

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hello Gabor, The change looks harmless so I think it can be accepted even without tests. Did you encountered the issue while analyzing some real code? Repository: rC Clang https://r

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

This revision was automatically updated to reflect the committed changes. Closed by commit rC345545: [ASTImporter] Reorder fields after structure import is finished (authored by a.sidorin, committed by ). Changed prior to commit: https://reviews.llvm.org/D44100?vs=160477&id=171578#toc Reposito

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Oops. Thank you Davide! Repository: rC Clang https://reviews.llvm.org/D44100 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thank you for the explanation! Repository: rC Clang https://reviews.llvm.org/D53697 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D50672: [ASTImporter] Change the return result of Decl import to Optional

a_sidorin abandoned this revision. a_sidorin added a comment. Another version has been committed (https://reviews.llvm.org/D51633). Repository: rC Clang https://reviews.llvm.org/D50672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

a_sidorin added a comment. Hi Gabor, Thank you for the patch. The reason for this change looks clear. However, I don't think omitting this comparison completely is what we want here. Instead, we can do a dance similar to `ASTContext::mergeFunctionTypes()` where all attributes but NoReturn are c

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a.sidorin added a comment. Hello everyone. @martong : this version of patch reorders only fields and friends. No method reordering is done (I can check if it works, though, and add the feature). @davide: Unfortunately, I was not able to reproduce the problem on Linux. Could you please provide a

[PATCH] D56581: [ASTImporter] Set the described template if not set

a_sidorin added a comment. Hello Gabor, This looks good, but as Shafik mentioned, adding a test would be nice. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56581/new/ https://reviews.llvm.org/D56581 ___ cfe-commits

[PATCH] D56651: [ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl

a_sidorin added a comment. Hi Rafael, The change looks mostly fine but I have some comments inline. Comment at: lib/AST/ASTImporter.cpp:3243 + + if (R) { +CXXDestructorDecl *ToDtor = cast(*R); It's better to move this code to VisitFunctionDecl to keep all

[PATCH] D56632: [analyzer] Track region liveness only through base regions.

a_sidorin added a comment. Hi Artem, This looks perfect, just some stylish issues. Comment at: test/Analysis/symbol-reaper.cpp:13 + void foo() { +// Ugh, maybe just let clang_analyzer_eval() work within callees already? +// The glob variable shouldn't keep our symbol a

[PATCH] D56651: [ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl

a_sidorin added a comment. Hi Shafik, Please find my answers inline. Comment at: lib/AST/ASTImporter.cpp:3243 + + if (R) { +CXXDestructorDecl *ToDtor = cast(*R); shafik wrote: > a_sidorin wrote: > > It's better to move this code to VisitFunctionDecl to kee

[PATCH] D56651: [ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thank you! I'm sorry if the change I requested doesn't look good, but I think the correctness is our priority. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56651/new/ ht

[PATCH] D57062: [analyzer] Re-enable the "System is over constrained" assertion on optimized builds.

a_sidorin accepted this revision. a_sidorin added a comment. Oh, I remember how much pain have debug mode-only assertions caused. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57062/new/ https://reviews.llvm.org/D57062 _

[PATCH] D57235: [AST] Add structural eq tests for template args

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. There are never enough tests :) Thank you! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57235/new/ https://reviews.llvm.org/D57235 __

[PATCH] D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter.

a_sidorin added a comment. Herald added a reviewer: shafik. Herald added a subscriber: gamesh411. Hi Balasz, As I guess, the next step is to convert all `Import`calls to `Import_New` and then rename `Import_New` into `Import`. If so, why aren't we able to just change the behaviour of `Import` m

[PATCH] D53702: [ASTImporter] Set redecl chain of functions before any other import

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Herald added a reviewer: shafik. Herald added a subscriber: gamesh411. Hi Gabor, The change looks fine. Thanks! Repository: rC Clang https://reviews.llvm.org/D53702 ___

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin updated this revision to Diff 174545. a_sidorin added a comment. Hi @davide and @shafik, Could you please check the updated version of the patch? Repository: rC Clang https://reviews.llvm.org/D44100 Files: lib/AST/ASTImporter.cpp unittests/AST/ASTImporterTest.cpp Index: unitte

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Hi Davide, I don't mean only review. As I guess, you guys have MacOS machines so you can check if the problem is still present in the updated version. There is no need to remind me about the problem with LLDB since I tried to resolve it. Repository: rC Clang http

[PATCH] D53708: [ASTImporter] Add importer specific lookup

a_sidorin added a subscriber: jingham. a_sidorin added a comment. Hi Gabor, I think it is a good patch with a nice test set. There are some mine comments inline. I'd also like to have LLDB guys opinion on it. Comment at: lib/AST/ASTImporter.cpp:7605 + +ASTImporter::ASTImporte

[PATCH] D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM. Thank you for addressing my questions! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53818/new/ https://reviews.llvm.org/D53818

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

a_sidorin added a comment. Hi Gabor, The change looks mostly Ok but there are some comments inline. Comment at: include/clang/AST/DeclContextInternals.h:128 +DeclsTy &Vec = *getAsVector(); +DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D); +return I != Ve

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

a_sidorin added a subscriber: spyffe. a_sidorin added a comment. Hi Balazs, The changes look mostly fine to me. I think they can be accepted after some minor stylish fixes. Hi Shafik, > I think these changes make sense at a high level but I am not sure about the > refactoring strategy. I am e

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

a_sidorin added subscribers: aaron.ballman, rsmith. a_sidorin added a comment. Please note that changes in AST lib will require AST code owners' approval. @rsmith, @aaron.ballman could you please take a look? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53655/ne

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

a_sidorin added a comment. More constants are always welcome. Comment at: lib/AST/ExternalASTMerger.cpp:154 ToContainer->setHasExternalLexicalStorage(); - ToContainer->setMustBuildLookupTable(); + ToContainer->getPrimaryContext()->setMustBuildLookupTable();

[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53693/new/ https://reviews.llvm.org/D53693 ___ cfe-c

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53751/new/ https://reviews.llvm.org/D53751 ___

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Hi Adrian, I've already done check-all (this includes LLDB) with this patch. As you can see in this thread, I didn't manage to reproduce some issues on Linux. Luckily, one test still failed on Linux so I could find the problem. However , it will be good to ensure that

[PATCH] D53708: [ASTImporter] Add importer specific lookup

a_sidorin added a comment. Hi Gabor, Here are some new comments. Comment at: lib/AST/ASTImporter.cpp:7658 +ASTImporter::FoundDeclsTy +ASTImporter::FindDeclsInToCtx(DeclContext *DC, DeclarationName Name) { + // We search in the redecl context because of transparent contexts. --

[PATCH] D53708: [ASTImporter] Add importer specific lookup

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, I think the code looks good. Thank you! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53708/new/ https://reviews.llvm.org/D53708 ___

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Hi Gabor, Thank you for digging into this! Unfortunately, I don't have any MacOS device and I cannot check my code for the compatibility. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100 _

[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699 ___

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

a_sidorin accepted this revision. a_sidorin added a comment. LGTM with a nit: the call to GetAlreadyImportedOrNull is not removed but moved into ImportDeclParts. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53755/new/ https://reviews.llvm.org/D53755

[PATCH] D55129: [CTU] Eliminate race condition in CTU lit tests

a_sidorin added a comment. Hi Gabor, Yes, the `%T` variable is obsolete, but it looks like the Guide recommendations are not entirely fulfilled. Comment at: test/Analysis/ctu-main.cpp:1 -// RUN: mkdir -p %T/ctudir -// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %

[PATCH] D55132: [CTU] Add asserts to protect invariants

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. More assertions are always good. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55132/new/ https://reviews.llvm.org/D55132

[PATCH] D55133: [CTU] Add statistics

a_sidorin added a comment. Hi Gabor, Sorry, but I don't understand the meaning of some options. Could you please explain what are NumNoUnit and NumNotInOtherTU and what is the difference between them? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55133/new/ htt

[PATCH] D55131: [CTU] Add more lit tests and better error handling

a_sidorin added a comment. Hi Gabor, Please find my comments inline. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:251 + cast_or_null(Importer.Import(const_cast(FD))); + if (!ToDecl) { +return llvm::make_error(index_error_code::failed_import); Cond

[PATCH] D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch

a_sidorin added a comment. Hi Gabor, In addition to Umann remarks, there is a small comment inline. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:239 + if (DisplayCTUProgress) { +llvm::errs() << "ANALYZE (CTU loaded AST for source file): " +

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

a_sidorin added a comment. Hi Gabor, Please find my comments inline. > Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be > an issue there? That's a good question. In Samsung, CTU hasn't been tested on ObjC code. Upstream CTU supports only FunctionDecls as well so it

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134 ___

[PATCH] D55289: [analyzer] MoveChecker Pt.5: Improve invalidation policies.

a_sidorin added a comment. Hi Artem, The change looks fine, just some nits. Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:537 +const MemRegion *ThisRegion = nullptr; +if (const auto *IC = dyn_cast_or_null(Call)) + ThisRegion = IC->getCXXThisVal().getAsReg

[PATCH] D55280: [CTU] Make loadExternalAST return with non nullptr on success

a_sidorin requested changes to this revision. a_sidorin added a comment. This revision now requires changes to proceed. Hi Gabor, There is a code in getExternalAST: std::unique_ptr LoadedUnit(ASTUnit::LoadFromASTFile( ASTFileName, CI.getPCHContainerOperations()->getRawReader(), ASTU

[PATCH] D55133: [CTU] Add statistics

a_sidorin accepted this revision. a_sidorin added a comment. LGTM with a nit. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:171 loadExternalAST(LookupFnName, CrossTUDir, IndexName); - if (!ASTUnitOrError) + if (!ASTUnitOrError) { return ASTUnitOrError.takeError

[PATCH] D55289: [analyzer] MoveChecker Pt.5: Improve invalidation policies.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LG! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55289/new/ https://reviews.llvm.org/D55289 ___ cfe-commits mailing list cfe-comm

[PATCH] D55307: [analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.

a_sidorin accepted this revision. a_sidorin added a comment. I think the change is fine, just a minor stylish remark. Comment at: test/Analysis/Inputs/system-header-simulator-cxx.h:782 +namespace std { + template // TODO: Implement the stub for deleter. + class unique_ptr { -

[PATCH] D55387: [analyzer] MoveChecker Pt.7: NFC: Misc refactoring.

a_sidorin added a comment. Hi Artem, The overall idea is good but I have some remarks inline. Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:265 +void MoveChecker::checkUse(ProgramStateRef State, const MemRegion *Region, + const CXXRecordDec

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Hi Gabor, Thank you for the investigation! I'll try to take a look. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100 ___ cfe-commits mailing list cfe

[PATCH] D57590: [ASTImporter] Improve import of FileID.

a_sidorin added a comment. Hi Gabor, Sorry, missed the patch. It looks mostly good, but do we have any test for this? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57590/new/ https://reviews.llvm.org/D57590 ___ cfe-c

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks for the fixes! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58494/new/ https://reviews.llvm.org/D58494 ___

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

a_sidorin added a comment. Hi Gabor, The patch looks almost good bu I have some comments inline. Comment at: lib/AST/ASTImporter.cpp:3002 + // Check if we have found an existing definition. Returns with that + // definition if yes, otherwise returns null.

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, Thanks for the patch! It looks good to me except some stylish nits. Comment at: lib/AST/ASTImporter.cpp:5130 +Importer.MapImported(D, PrevDecl->getDef

[PATCH] D55358: [ASTImporter] Fix import of NestedNameSpecifierLoc.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Looks good! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55358/new/ https://reviews.llvm.org/D55358

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58668/new/ https://reviews.llvm.org/D58668 ___ cfe-c

[PATCH] D58897: [ASTImporter] Make ODR error handling configurable

a_sidorin added a comment. Hi Gabor, This patch LGTM mostly, but there is a comment inline. Comment at: include/clang/AST/ASTStructuralEquivalence.h:81 EqKind(EqKind), StrictTypeSpelling(StrictTypeSpelling), -ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch), Comp

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

a_sidorin added a subscriber: davide. a_sidorin added a comment. Hi Shafik, Honestly, I was always wondering what does HandleNameConflict actually do. Its implementation in ASTImporter is trivial and I don't see any of its overrides in LLDB code too. Why do we check its result to be non-empty i

[PATCH] D59692: [ASTImporter] Fix name conflict handling

a_sidorin added a comment. Hi Gabor, Thank you for addressing the problem! Comment at: lib/AST/ASTImporter.cpp:2256 return Importer.MapImported(D, FoundTypedef); -} -// FIXME Handle redecl chain. -break; +} else + Conflicti

[PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

a_sidorin added a comment. Hello Raphael, Thank you for the explanation. I have +1 to Gabor's question to understand if this functionality can actually be added to the common ASTImporter. Comment at: clang/include/clang/AST/ASTImporter.h:149 +/// decl on its own. +vir

[PATCH] D53757: [ASTImporter] Changed use of Import to Import_New in ASTNodeImporter.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Balasz, Sorry, I missed the review accidentally. Thank you for the patch! Comment at: lib/AST/ASTImporter.cpp:3418 - for (const auto *Attr : D->attrs()) -ToI

[PATCH] D55049: Changed every use of ASTImporter::Import to Import_New

a_sidorin added a comment. Hi Balazs, The looks mostly good to me. Comment at: lib/AST/ASTImporter.cpp:3440 - for (const auto *Attr : D->attrs()) -ToIndirectField->addAttr(Importer.Import(Attr)); There is the same deletion in D53757.

[PATCH] D59692: [ASTImporter] Fix name conflict handling

a_sidorin added a comment. Thanks for the fixes! I'd like to clarify one moment, however. Comment at: lib/AST/ASTImporter.cpp:2154 +return NameOrErr.takeError(); } } Should we write `else Name = *NameOrError`? Repository: rC Clang CHANGES S

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

a_sidorin added a comment. Hi Shafik, Thank you for the explanation, it is much more clear to me now. But, as I see, D59692 is going to discard the changes this patch introduces. @martong Gabor, do you expect the changes of this patch to be merged into yours, o

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

a_sidorin added a comment. Post-LGTM with some stylish nits. Comment at: cfe/trunk/lib/AST/ASTImporter.cpp:1950 + // Eliminate a potential failure point where we attempt to re-import + // something we're trying to import while completin ToEnum + if (Decl *ToOrigin = Importer

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

a_sidorin accepted this revision. a_sidorin added a comment. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59665/new/ https://reviews.llvm.org/D59665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D59761: [ASTImporter] Convert ODR diagnostics inside ASTImporter implementation

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Yes, I think this is fine. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59761/new/ https://reviews.llvm.org/D59761 __

[PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hello Raphael, I think we should accept this change. I don't see an easy way to merge the LLDB stuff into ASTImporter; also, this patch provides a good extension point for ASTImporter si

[PATCH] D55049: Changed every use of ASTImporter::Import to Import_New

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55049/new/ https://reviews.llvm.org/D55049 ___ cfe-com

[PATCH] D60739: [analyzer] NFC: Re-use reusable unittest mocks.

a_sidorin accepted this revision. a_sidorin added a comment. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60739/new/ https://reviews.llvm.org/D60739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

a_sidorin accepted this revision. a_sidorin added a comment. I like the test even more than the change itself! Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:49 +// Bind(Zero) +Store StX0 = +StMgr.Bind(StInit, LX0, Zero).getStore(); Th

<    1   2   3   4   5   >