[PATCH] D53979: [analyzer][CTU] Correctly signal in the function index generation tool if there was an error

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Looks good to me. It is great to see that we can get rid of so many header and lib dependencies. https://reviews.llvm.org/D53979 ___

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

martong marked 2 inline comments as done. martong added a comment. > 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.

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

martong updated this revision to Diff 171723. martong marked 2 inline comments as done. martong added a comment. - Remove unrelated test Repository: rC Clang https://reviews.llvm.org/D53697 Files: lib/AST/ASTStructuralEquivalence.cpp unittests/AST/ASTImporterTest.cpp Index:

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

martong created this revision. martong added a reviewer: a_sidorin. Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. When we already have an incomplete underlying type of a typedef in the "To" context, and the "From" context has the same

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

martong created this revision. martong added a reviewer: a_sidorin. Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs. martong added a dependency: D53693: [ASTImporter] Typedef import brings in the complete type. If one definition is currently being defined, we do not compare

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

martong created this revision. martong added a reviewer: a_sidorin. Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. FunctionType::ExtInfo holds such properties of a function which are needed mostly for code gen. We should not compare these

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

martong created this revision. martong added a reviewer: a_sidorin. Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. FunctionDecl import starts with a lookup and then we create a new Decl. Then in case of CXXConstructorDecl we further import

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

martong created this revision. martong added a reviewer: a_sidorin. Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. During method import we check for structural eq of two methods. In the structural eq check we check for their isVirtual()

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

martong created this revision. martong added a reviewer: a_sidorin. Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs, mgorny. Herald added a reviewer: a.sidorin. There are certain cases when normal C/C++ lookup (localUncachedLookup) does not find AST nodes. E.g.: Example 1:

[PATCH] D51533: [ASTImporter] Merge ExprBits

martong added inline comments. Comment at: unittests/AST/ASTImporterTest.cpp:3241 + auto *ToD = Import(FromD, Lang_CXX11); + ASSERT_TRUE(ToD); + auto *ToInitExpr = cast(ToD)->getAnyInitializer(); a_sidorin wrote: > EXPECT_TRUE (same below). Thanks, changed

[PATCH] D51533: [ASTImporter] Merge ExprBits

This revision was automatically updated to reflect the committed changes. martong marked an inline comment as done. Closed by commit rL341316: [ASTImporter] Merge ExprBits (authored by martong, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D51597: [ASTImporter] Fix import of VarDecl init

martong created this revision. martong added reviewers: a_sidorin, xazax.hun, r.stahl. Herald added subscribers: cfe-commits, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a reviewer: a.sidorin. The init expression of a VarDecl is overwritten in the "To" context if we import

[PATCH] D51597: [ASTImporter] Fix import of VarDecl init

martong added inline comments. Comment at: unittests/AST/ASTImporterTest.cpp:3763 +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods, +DefaultTestValuesForRunOptions, ); This hunk has nothing to do with this change, but

[PATCH] D56936: Fix handling of overriden methods during ASTImport

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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

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 =

[PATCH] D56936: Fix handling of overriden methods during ASTImport

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

[PATCH] D55646: [ASTImporter] Make ODR diagnostics warning by default

martong added a comment. The primary reason why want these to be Warnings instead of Errors because there are many false positive ODR Errors at the moment. These are not fatal errors but there is a maximum error count which once reached the whole process (analysis) is aborted. Usually, such

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

martong added a comment. @shafik > We need to make sure we are running the lldb test suite before committing and > watching the bots to make sure the commit does not break them. I just have tested this patch on macOS and there seems to be no regression. Linux also seems to be without any

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

martong updated this revision to Diff 178060. martong marked 2 inline comments as done. martong added a comment. - Add more tests and simplify comment Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699 Files:

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

martong marked 4 inline comments as done. martong added inline comments. Comment at: lib/AST/ASTStructuralEquivalence.cpp:302 +/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling +/// conventions bits but must not compare some other bits, e.g. the noreturn

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

martong added a comment. Alexey, good news! I have finished the testing of the above patch applied on top of 174545. Neither is a regression on Linux nor on macOS. (ASTTests, check-clang-astmerge, check-clang-import, check-clang-analysis, check-lldb). Repository: rC Clang CHANGES SINCE

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL348923: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull (authored by martong, committed by ).

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

martong added a comment. Ping @shafik, I have addressed you comments, could you please take another look? If you don't have any objections, could you please approve? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699

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

martong added a comment. Ping @a_sidorin Could you please take another look and consider my previous comment? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55280/new/ https://reviews.llvm.org/D55280 ___ cfe-commits

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL349351: [ASTImporter] Add importer specific lookup (authored by martong, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC349349: [ASTImporter] Fix redecl chain of classes and class templates (authored by martong, committed by ). Changed prior to commit: https://reviews.llvm.org/D53655?vs=175641=178450#toc Repository:

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

martong updated this revision to Diff 178452. martong added a comment. - Rebase to master (trunk) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53708/new/ https://reviews.llvm.org/D53708 Files: include/clang/AST/ASTImporter.h

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

martong added a comment. > This is a perhaps a naive comment, but why is localUncachedLookup used instead of noload_lookup ? There is a fixme dating from 2013 stating that noload_lookup should be used instead. This is not a naive question. I was wondering myself on the very same thing when I

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

martong added a comment. Alexey, simply put, the crux of the problem is this: The imported decl context is different than what we get by calling the `getDeclContext()` on an imported field/friend of the "from" decl context. With other words: `import(cast(FromDC)` gives us a different value than

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

martong added a comment. In D53708#1332922 , @riccibruno wrote: > In D53708#1332868 , @martong wrote: > > > > This is a perhaps a naive comment, but why is localUncachedLookup used > > > instead of noload_lookup ?

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

martong added a comment. Hi Rafael, This is a great patch and good improvement, thank you! I had a few minor comments. (Also, I was thinking about what else could we import in the future, maybe definitions of C++11 enum classes would be also worth to be handled by CTU.)

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

martong created this revision. martong added reviewers: xazax.hun, a_sidorin. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Repository: rC Clang https://reviews.llvm.org/D55132 Files: lib/CrossTU/CrossTranslationUnit.cpp Index:

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

martong created this revision. martong added reviewers: xazax.hun, a_sidorin. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. We introduce a strict policy for C++ CTU. It can work across TUs only if the C++ dialects are the same. We neither allow C vs C++ CTU. We

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

martong created this revision. martong added reviewers: xazax.hun, a_sidorin. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. We plan to introduce additional CTU related lit test. Since lit may run the tests in parallel, it is not safe to use the same directory (%T)

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

martong created this revision. martong added reviewers: xazax.hun, balazske, a_sidorin. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Adding some more CTU list tests. E.g. to check if a construct is unsupported. We also slightly modify the handling of the return

[PATCH] D55133: [CTU] Add statistics

martong created this revision. martong added reviewers: xazax.hun, a_sidorin. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Repository: rC Clang https://reviews.llvm.org/D55133 Files: lib/CrossTU/CrossTranslationUnit.cpp Index:

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

martong created this revision. martong added reviewers: xazax.hun, Szelethus, a_sidorin. Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity. Herald added a reviewer: george.karpenkov. With a new

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

martong updated this revision to Diff 175641. martong marked 2 inline comments as done. martong added a comment. - Remove containsInVector Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53655/new/ https://reviews.llvm.org/D53655 Files:

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

martong added a comment. @aaron.ballman Thanks for your review. I have updated the patch to remove `containsInVector`. > This seems mostly reasonable to me, but @rsmith is more well-versed in this > area and may have further comments. You should give him a few days to chime > in before

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

martong updated this revision to Diff 175647. martong added a comment. - Use ExtInfo in structural equivalency Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699 Files: lib/AST/ASTStructuralEquivalence.cpp

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

martong added a subscriber: teemperor. martong added a comment. Hi @shafik , Thank you for your review. I have removed the call of `getPrimaryContext`. Fortunately, we already have one patch for the `getPrimaryContext` related changes, made by @teemperor : https://reviews.llvm.org/D54898 . I

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

martong updated this revision to Diff 175639. martong marked an inline comment as done. martong added a comment. - Remove call of 'getPrimaryContext' Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53755/new/ https://reviews.llvm.org/D53755 Files:

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

martong marked 4 inline comments as done. martong added inline comments. Comment at: include/clang/AST/DeclContextInternals.h:125-129 + bool containsInVector(const NamedDecl *D) { +assert(getAsVector() && "Must have a vector at this point"); +DeclsTy = *getAsVector();

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

martong added a comment. Hi @a_sidorin , I have updated the patch as you suggested, to check the equivalence based on `ASTContext::mergeFunctionTypes()`. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699

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

martong added a comment. > I didn't actually see this comment get addressed other than to say it won't > be a problem in practice, which I'm not certain I agree with. Was there a > reason why this got commit before finding out if the reviewer with the > concern agrees with your rationale?

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

martong updated this revision to Diff 176172. martong added a comment. - Add lit tests Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134 Files: include/clang/Basic/DiagnosticCrossTUKinds.td

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

martong updated this revision to Diff 176582. martong added a comment. - Change the CTU progress message in the test too Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55135/new/ https://reviews.llvm.org/D55135 Files:

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

martong updated this revision to Diff 176581. martong marked 4 inline comments as done. martong added a comment. - Rename AnalyzerDisplayCtuProgress to Opts.AnalyzerDisplayCTUProgress - Change the CTU progress message Repository: rC Clang CHANGES SINCE LAST ACTION

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

martong added a comment. > Also, almost everywhere CTU is capitalized, so I guess it should be in the > field name too. Ok, I renamed it to have CTU all capitalized. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:239 + if (DisplayCTUProgress) { +llvm::errs()

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

martong updated this revision to Diff 176590. martong marked an inline comment as done. martong added a comment. - Break long RUN line Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55135/new/ https://reviews.llvm.org/D55135 Files:

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

martong added inline comments. Comment at: test/Analysis/ctu-main.cpp:6 // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config

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

martong updated this revision to Diff 176584. martong added a comment. - Break long RUN lines Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134 Files: include/clang/Basic/DiagnosticCrossTUKinds.td

[PATCH] D55280: [CTU] Remove redundant error check

martong added a comment. @Szelethus @balazske Thanks for your review! The meantime I have discussed with @xazax.hun that actually the called `ASTUnit::LoadFromASTFile` function inside `loadExternalAST` may return with a nullptr. So, the best is to handle that inside `loadExternalAST`.

[PATCH] D55280: [CTU] Remove redundant error check

martong updated this revision to Diff 176779. martong added a comment. - Return an error from loadExternalAST in case of nullptr Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55280/new/ https://reviews.llvm.org/D55280 Files:

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

martong added a comment. > Also, maybe it'd be worth making a CTU directory under test/Analysis for CTU > related test files. It is a good point, but I'd do that in the future once we have even more ctu related test files. Perhaps together with a new check-clang-analysis-ctu build target.

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

martong updated this revision to Diff 176619. martong marked 8 inline comments as done. martong added a comment. - Break long RUN lines - Clang format the test files - Use consistent naming style - Remove braces Repository: rC Clang CHANGES SINCE LAST ACTION

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

martong added inline comments. Comment at: test/Analysis/Inputs/ctu-other.c:6 + int b; +} foobar; + a_sidorin wrote: > Please use a consistent naming style across the file. There are names > starting with capital, having underscores and written like this. Ok,

[PATCH] D55133: [CTU] Add statistics

martong updated this revision to Diff 176655. martong marked an inline comment as done. martong added a comment. - Remove braces Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55133/new/ https://reviews.llvm.org/D55133 Files:

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

martong updated this revision to Diff 176660. martong added a comment. - Use rm, mkdir and break long RUN lines Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55129/new/ https://reviews.llvm.org/D55129 Files: test/Analysis/ctu-main.cpp Index:

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

martong added a comment. @rsmith ping Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53655/new/ https://reviews.llvm.org/D53655 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

martong updated this revision to Diff 176672. martong added a comment. - Forgot to rename err_ctu_incompat_triple -> warn_ctu_incompat_triple Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134 Files:

[PATCH] D55133: [CTU] Add statistics

martong added a comment. > 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? Your point is absolutely true. They are the same, I think at some point some time ago they were

[PATCH] D55133: [CTU] Add statistics

martong updated this revision to Diff 176654. martong added a comment. - Remove NumNoUnit Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55133/new/ https://reviews.llvm.org/D55133 Files: lib/CrossTU/CrossTranslationUnit.cpp Index:

[PATCH] D55280: [CTU] Remove redundant error check

martong created this revision. martong added reviewers: xazax.hun, a_sidorin, Szelethus, balazske. Herald added subscribers: cfe-commits, gamesh411, dkrupp, rnkovacs. In loadExternalAST we normally return with either an error or with a valid ASTUnit pointer which should not be a nullptr. However,

[PATCH] D55133: [CTU] Add statistics

martong added a comment. >> 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? > Your point is absolutely true. They are the same, I think at some point some > time ago

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

martong added inline comments. Comment at: include/clang/Basic/DiagnosticCrossTUKinds.td:19 + +def err_ctu_incompat_triple : Error< + "imported AST from '%0' had been generated for a different target, current: %1, imported: %2">; xazax.hun wrote: > I am not

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

martong updated this revision to Diff 176671. martong marked 4 inline comments as done. martong added a comment. - Diagnose a warning (which may be upgradable to an error) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134

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

martong updated this revision to Diff 176653. martong added a comment. - Use clang_analyze_cc1 - Change to be an analyzer config option Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55135/new/ https://reviews.llvm.org/D55135 Files:

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

martong added a comment. > should be an -analyzer-config option. Ok, just changed it to be. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55135/new/ https://reviews.llvm.org/D55135 ___ cfe-commits mailing list

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

martong marked an inline comment as done. martong added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:147 llvm::Expected CrossTranslationUnitContext::getCrossTUDefinition(const FunctionDecl *FD, balazske wrote: > Szelethus wrote: > >

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

martong marked an inline comment as done. martong added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:247 llvm::Expected CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) { ASTImporter = getOrCreateASTImporter(FD->getASTContext());

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

martong marked 2 inline comments as done. martong added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:212 +// diagnostics. +Context.getDiagnostics().Report(diag::err_ctu_incompat_triple) +<< Unit->getMainFileName() << TripleTo.str() <<

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

martong updated this revision to Diff 176687. martong added a comment. - Use clang_analyze_cc1 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134 Files: include/clang/Basic/DiagnosticCrossTUKinds.td

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

martong added a comment. > We should probably prefer %clang_analyze_cc1 to %clang_cc1 -analyze here too. Ok, changed that. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134

[PATCH] D55133: [CTU] Add statistics

This revision was automatically updated to reflect the committed changes. Closed by commit rL348584: [CTU] Add statistics (authored by martong, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D55133?vs=176655=177173#toc Repository:

[PATCH] D55133: [CTU] Add statistics

martong marked 2 inline comments as done. martong added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:171 loadExternalAST(LookupFnName, CrossTUDir, IndexName); - if (!ASTUnitOrError) + if (!ASTUnitOrError) { return ASTUnitOrError.takeError();

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL348586: [CTU] Add asserts to protect invariants (authored by martong, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

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

martong marked 2 inline comments as done. martong added inline comments. Comment at: test/Analysis/ctu-main.c:4 +// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %t/ctudir2/ctu-other.c.ast %S/Inputs/ctu-other.c +// RUN: cp %S/Inputs/externalFnMap2.txt

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

martong updated this revision to Diff 177170. martong marked an inline comment as done. martong added a comment. - Rename externalFnMap files Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55131/new/ https://reviews.llvm.org/D55131 Files:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL348587: [CTU] Eliminate race condition in CTU lit tests (authored by martong, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION

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

martong marked an inline comment as done. martong added a comment. > While Static Analyzer is the only client of CTU library at the moment, we > might have more in the future. I would not use the phrase ANALYZE in the log > message. Once this is resolved the rest looks good. Ok, I removed the

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

martong updated this revision to Diff 177204. martong added a comment. - Remove 'ANALYZE ' prefix from the log message Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55135/new/ https://reviews.llvm.org/D55135 Files: include/clang/CrossTU/CrossTranslationUnit.h

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL348605: [CTU] Add more lit tests and better error handling (authored by martong, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL348610: [CTU] Add triple/lang mismatch handling (authored by martong, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL348594: [CTU] Add DisplayCTUProgress analyzer switch (authored by martong, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

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

martong updated this revision to Diff 176417. martong marked 13 inline comments as done. martong added a comment. - Address review comments Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134 Files:

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

martong added a comment. > Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be > an issue there? I really don't know. We never tried it, our focus is on C and C++ for now. Unfortunately, there is nobody with ObjC/C++ knowledge and (more importantly) with interest to

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

martong added a comment. Thanks for the review! I have updated the patch according to your comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:31 +namespace llvm { +// Same as Triple's equality operator, but we check a field only if that is a_sidorin

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

martong added a comment. Alexei, I had a chance to have a quick look into this on a borrowed Mac Book. Seems like the assertion which was related to the revert is not existent anymore, but a new assertion came in. Next week I'll have time to debug that and I'll come back to you with what I

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

martong added a comment. Hi Aleksey, The first version was indeed not correct. But, I think we handle that case with the latest update (https://reviews.llvm.org/differential/diff/176779/). In `loadExternalAST()` we have this right before the return: if (!Unit) return

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

martong added a comment. @rsmith Please raise any objections until Dec 14 (or if this deadline is too strict). I'd like to commit this next week latest so it can get in still this year. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53655/new/

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

martong added a comment. @a_sidorin The below diff on top of your patch successfully handles the failure with the `TestCModules.py` LLDB testcase: diff --git a/lib/AST/ASTImporter.cpp b/lib/AST/ASTImporter.cpp index 05fec7f943..e6fb590025 100644 --- a/lib/AST/ASTImporter.cpp +++

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

martong added a comment. There is another failure on macOS, which is not there on Linux. This is present with the 174545 patch id (even before applying my fix for TestCModules). $ ninja check-clang-astmerge Testing Time: 0.63s Failing Tests

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

martong updated this revision to Diff 176100. martong marked 3 inline comments as done. martong added a comment. - Address Alexei's comments - Rename FindDeclsInToCtx to findDeclsInToCtx - Remove superfluous spaces from stringliterals - Remove unused header - Remove empty test - Return nullptr

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

martong marked 8 inline comments as done. martong added inline 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

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

martong added a comment. Hey Alexei, Thank you again for your comments. I changed to code accordingly. Also, I recognized that we should use a `SmallSetVector` instead of a `SmallPtrSet`, because the latter results unpredictable iteration (pointers may have different values with each run).

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

martong updated this revision to Diff 176686. martong marked an inline comment as done. martong added a comment. - Add a case to emitCrossTUDiagnostics Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55134/new/ https://reviews.llvm.org/D55134 Files:

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

martong added a comment. Hey Alexey, Here is what I found so far: The new assertion is this: Assertion failed: (ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD)), function ImportDeclContext ToRD looks like this in code: typedef struct __sFILE { // several fields here ... }

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

martong updated this revision to Diff 175127. martong added a comment. - Use MostRecentDecl when setting PrevDecl Repository: rC Clang https://reviews.llvm.org/D53655 Files: include/clang/AST/DeclContextInternals.h include/clang/ASTMatchers/ASTMatchers.h lib/AST/ASTImporter.cpp

<    1   2   3   4   5   6   7   8   9   10   >