[PATCH] D49223: [AST] Check described template at structural equivalence check.

a.sidorin accepted this revision. a.sidorin added a comment. Hi Balazs, I have only two nits. Otherwise, the patch is OK and can be committed without additional approval after the comments are fixed. Thank you! Comment at: lib/AST/ASTStructuralEquivalence.cpp:1500 +bool Stru

[PATCH] D50428: [ASTImporter] Add support for importing imaginary literals

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM! Just a stylish nit. Comment at: lib/AST/ASTImporter.cpp:5617 + + return new (Importer.getToContext()) + ImaginaryLiteral(SubE, T); The line

[PATCH] D50444: [ASTImporter] Fix structural inequivalency of forward EnumDecl

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Yes, this seems to be correct. Thanks! Comment at: lib/AST/ASTStructuralEquivalence.cpp:1182 + + // Compare the definitions of these two eunums. If either or both are

[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

a_sidorin added a comment. Hi Gabor, While importing methods looks harmless, importing fields can be a breaking change. Do you have any test results on this? Comment at: lib/AST/ASTImporter.cpp:2872 Importer.MapImported(D, FoundField); +// In case of a FieldD

[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works

a_sidorin added a comment. Hello Richard, Thank you for clarification. However, I think that moving ParentMap into libTooling is out of scope for this patch. Are you OK if this change will be committed with adding a TODO or FIXME for this move? https://reviews.llvm.org/D46940 _

[PATCH] D49798: [ASTImporter] Adding some friend function related unittests.

a_sidorin added a comment. Hi Balazs, The patch looks mostly fine. Comment at: unittests/AST/ASTImporterTest.cpp:2256 +FirstDeclMatcher().match(FromTU, cxxRecordDecl()); +auto lookup_res = Class->noload_lookup(FromName); +ASSERT_EQ(lookup_res.size(), 0u); -

[PATCH] D50516: [ASTImporter] Improved import of friend templates.

a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: unittests/AST/ASTImporterTest.cpp:2721 + EXPECT_EQ(ToFriendClass->getDefinition(), ToClass); + ASSERT_EQ(ToFriendClass->getPreviousDecl(), ToClass); +

[PATCH] D50552: [ASTImporter] Added test case for CXXConversionDecl importing

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang https://reviews.llvm.org/D50552 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.o

[PATCH] D50550: [ASTImporter] Added test case for opaque enums

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D50550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

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

a_sidorin updated this revision to Diff 160477. a_sidorin edited the summary of this revision. a_sidorin added a comment. All declarations are reordered now, not only fields. Also some review comments were addressed. Repository: rC Clang https://reviews.llvm.org/D44100 Files: lib/AST/ASTI

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

a_sidorin marked 2 inline comments as done. a_sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:1029 + + RecordDecl *ToRD = cast(Importer.Import(cast(FromDC))); + martong wrote: > Can't we just import the `FromRD`, why we need that cast at the e

[PATCH] D50731: [ASTImporter] Add test for ExprWithCleanups

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Tests are always welcome. Thanks! Repository: rC Clang https://reviews.llvm.org/D50731 ___ cfe-commits mailing list cfe-commits@lists.ll

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

a_sidorin marked an inline comment as done. a_sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:1317 + for (auto *D : FromRD->decls()) { +Decl *ToD = Importer.GetAlreadyImportedOrNull(D); +assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));

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

a_sidorin added a comment. Hello Gabor and Balazs, > With Balazs, we are working on something similar, but with a different, fine > grained error value mechanism. Unfortunately we were not aware of that you > have been working on error handling, and we didn't say that we are working on > error

[PATCH] D49798: [ASTImporter] Adding some friend function related unittests.

a_sidorin added a comment. Hi Balasz, Thank you for the fixes. Comment at: unittests/AST/ASTImporterTest.cpp:2258 +ASSERT_EQ(lookup_res.size(), 0u); +lookup_res = cast(FromTU)->noload_lookup(FromName); +ASSERT_EQ(lookup_res.size(), 1u); a_sidorin w

[PATCH] D50732: [ASTImporter] Add test for CXXDefaultInitExpr

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! > As a side note: It seems this test case actually reveals that we don't import > the body of Foo's destructor? This is strange. If you manage to find the reason, please notify

[PATCH] D50737: [ASTImporter] Add test for CXXNoexceptExpr

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang https://reviews.llvm.org/D50737 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.o

[PATCH] D50932: [ASTImporter] Add test for C++ casts and fix broken const_cast importing.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thank you! Comment at: tools/clang-import-test/clang-import-test.cpp:197 Inv->getLangOpts()->DollarIdents = true; + Inv->getLangOpts()->RTTI = true; Inv->getCode

[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

a_sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:4550 + // in the "From" context, but not in the "To" context. + for (auto *FromField : D->fields()) +Importer.Import(FromField); martong wrote: > martong wrote: > > a_sidorin w

[PATCH] D50978: [ASTImporter] Add test for C++'s try/catch statements.

a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: tools/clang-import-test/clang-import-test.cpp:199 Inv->getLangOpts()->RTTI = true; + Inv->getLangOpts()->Exceptions = true; + Inv->getLangOpts()->C

[PATCH] D50662: Add dump() method for SourceRange

a_sidorin requested changes to this revision. a_sidorin added a comment. This revision now requires changes to proceed. Hello Stephen, These methods will be really useful. Comment at: lib/Basic/SourceLocation.cpp:90 + B.print(OS, SM); + OS << ",\n "; + E.print(OS, SM); -

[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

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

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

a.sidorin added subscribers: NoQ, a.sidorin. a.sidorin added a comment. Accidentally noticed about this review via cfe-commits. @NoQ, the change in the ExprEngine looks a bit dangerous to me. Could you please check? Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:419 +

[PATCH] D51115: [ASTImporter] Actually test ArrayInitLoopExpr in the array-init-loop-expr test.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Wow, I totally overlooked this. Thank you! Repository: rC Clang https://reviews.llvm.org/D51115 ___ cfe-commits mailing list cfe-commits

[PATCH] D51056: [ASTImporter] Add test for SwitchStmt

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

[PATCH] D46398: [ASTImporterTest] Fix potential use-after-free

a.sidorin updated this revision to Diff 146587. a.sidorin added a comment. After a number of attempts of Twine'ifying all Code samples, I decided to restore the initial state of this code. Repository: rC Clang https://reviews.llvm.org/D46398 Files: unittests/AST/ASTImporterTest.cpp Inde

[PATCH] D46398: [ASTImporterTest] Fix potential use-after-free

a.sidorin updated this revision to Diff 146589. a.sidorin added a comment. Add forgotten context. Repository: rC Clang https://reviews.llvm.org/D46398 Files: unittests/AST/ASTImporterTest.cpp Index: unittests/AST/ASTImporterTest.cpp ===

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

a.sidorin added a comment. Gentle ping. Repository: rC Clang https://reviews.llvm.org/D45416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D46353#1086456, @martong wrote: > > should we add this new declaration to the redeclaration chain like we do it > > for RecordDecls? > > I think, on a long te

[PATCH] D46398: [ASTImporterTest] Fix potential use-after-free

a.sidorin closed this revision. a.sidorin added a comment. Closed with https://reviews.llvm.org/rC332256. Repository: rC Clang https://reviews.llvm.org/D46398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/m

[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

a.sidorin added a comment. Hi Gabor, I don't feel I'm a right person to review AST-related part so I'm adding other reviewers. What I'm worrying about is that there is no test to check if our changes in removeDecl are correct. Maybe https://reviews.llvm.org/D44100 is a right patch for this but

[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates

This revision was automatically updated to reflect the committed changes. Closed by commit rC332338: [ASTImporter] Extend lookup logic in class templates (authored by a.sidorin, committed by ). Changed prior to commit: https://reviews.llvm.org/D46353?vs=146765&id=146780#toc Repository: rC Cl

[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence

a.sidorin added a comment. Hello Gabor! Thank you for this patch! Do you plan to enable this functionality for AST import checking? Some comments are inline. Comment at: unittests/AST/Language.h:1 +//===- unittest/AST/Language.h - AST unit test support ---===// +/

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

a.sidorin added a comment. Ping. 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] D46940: [ASTImporter] make sure that ACtx::getParents still works

a.sidorin added a comment. Hello Rafael, The patch is awesome. The only thing I'm uncomfortable with is that we call invalidation on every stmt import - not only during the top-level one. Fixing this requires separating entry point from internal imports and is out of scope of this patch, but a

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

a.sidorin added a comment. This is a nice extension of https://reviews.llvm.org/D16063. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSiz

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

a.sidorin added inline comments. Comment at: test/Analysis/array-index.c:11 + +void fie() { + buf[SIZE-1] = 1; Could you please give meaningful names to the tests? Repository: rC Clang https://reviews.llvm.org/D46944 _

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

a.sidorin added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344 // This is a signed value, since it's used in arithmetic with signed indices. - return svalBuilder.makeIntVal(RegionSize / EleSize, false); + return svalBuilder.makeIntVal(RegionSize /

[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem

a.sidorin added a comment. Hello Gabor, The patch is fine, I have found only some style issues. Comment at: lib/AST/ASTImporter.cpp:4073 +// Returns the definition for a (forwad) declaration of a ClassTemplateDecl, if +// it has any definition in the redecl chain. --

[PATCH] D46958: [ASTImporter] Fix missing implict CXXRecordDecl

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. So, we fail to add injected name to a CXXRecordDecl that has a described class template? Nice catch! LGTM. Repository: rC Clang https://reviews.llvm.org/D46958 ___

[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works

a.sidorin accepted this revision. a.sidorin added a comment. LGTM. Aaron, could you please confirm that AST changes are fine? Comment at: include/clang/AST/ASTContext.h:638 +ReleaseParentMapEntries(); +PointerParents = nullptr; + } I'd prefer to call r

[PATCH] D47069: [ASTImporter] Enable disabled but passing test

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Nice! Repository: rC Clang https://reviews.llvm.org/D47069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor! I have a question. Comment at: lib/AST/ASTImporter.cpp:4305 +// Add to the DC only if it was an explicit specialization/instantiation. +if (D2->getTem

[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

a.sidorin requested changes to this revision. a.sidorin added a comment. This revision now requires changes to proceed. (Sorry, accepted accidentially). Repository: rC Clang https://reviews.llvm.org/D47058 ___ cfe-commits mailing list cfe-commits

[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

a.sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:1962 TagDecl *Definition = D->getDefinition(); - if (Definition && Definition != D) { + if (!D->isImplicit() && Definition && Definition != D) { Decl *ImportedDef = Importer.Import(Definition); ---

[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang https://reviews.llvm.org/D46950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. LGTM with a nit. Comment at: lib/AST/ASTImporter.cpp:1962 TagDecl *Definition = D->getDefinition(); - if (Definition && Definition != D) { + if (!D->isImplicit() /

[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

a.sidorin added a comment. Hi Gabor, Could you add a test for TSK_Undeclared as well? Repository: rC Clang https://reviews.llvm.org/D47058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

a.sidorin added a comment. Hi Bevin, Could you please address these comments? Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89 SymMgr(context, BasicVals, alloc), MemMgr(context, alloc), -StateMgr(stateMgr), ArrayIndexTy(context.Long

[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, LGTM, thank you for addressing the comments! Just a minor nit, it's OK to fix it before committing without a separate review. Comment at: unittests/AST/Stru

[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Ok, I got it, thank you! Repository: rC Clang https://reviews.llvm.org/D47058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem

a.sidorin added a comment. Hm. Should we test `-fms-compatibility` in addition to `-fdelayed-template-parsing`? Repository: rC Clang https://reviews.llvm.org/D46950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cg

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

a.sidorin added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89 SymMgr(context, BasicVals, alloc), MemMgr(context, alloc), -StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy), +StateMgr(stateMgr), ArrayIndex

[PATCH] D47445: [ASTImporter] Corrected diagnostic client handling in tests.

a.sidorin accepted this revision. a.sidorin added a comment. Looks good to me, but the approval from AST code owners is required, I think. Repository: rC Clang https://reviews.llvm.org/D47445 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

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

a.sidorin requested changes to this revision. a.sidorin added a comment. This revision now requires changes to proceed. Hello Balázs! Thank you for addressing this problem, it is really cool. However, it will be much better if we don't suppress warnings but fix them. Could you modify the tests

[PATCH] D47367: [ASTImporter] Add ms compatibility to tests which use the TestBase

a.sidorin added a comment. > I think, it is overkill to test all possible combinations of the options, we > should come up with something better here. I agree with that. I think we need to test just import pairs {/*From*/no_option, /*To*/no_option}, {option_1, option1}, {option_2, option_2}, .

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

a.sidorin added a subscriber: NoQ. a.sidorin added a comment. There are some results for clang and gcc max value for x86 and x64. Source code: const unsigned long long SIZE_MAX = (unsigned long long)(unsigned long)(-1); const unsigned long long size = SIZE_MAX/2; char arr[size+1]; Compiler

[PATCH] D47313: [ASTImporter] Corrected lookup at import of templated record decl

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. LGTM too, thank you! Do you need someone to commit this for you? Repository: rC Clang https://reviews.llvm.org/D47313 ___ cfe-commits ma

[PATCH] D20118: Add support for injected class names and constructor initializers in C++

a.sidorin abandoned this revision. a.sidorin added a comment. This revision seems to be already committed in https://reviews.llvm.org/rC269693, without Differential Revision set. Repository: rL LLVM https://reviews.llvm.org/D20118 ___ cfe-commit

[PATCH] D47367: [ASTImporter] Add ms compatibility to tests which use the TestBase

a.sidorin added a comment. I meant that we can use this approach for testImport() too. Repository: rC Clang https://reviews.llvm.org/D47367 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe

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

a.sidorin requested changes to this revision. a.sidorin added a comment. This revision now requires changes to proceed. Hello, Balázs, You can find my comments inline. Comment at: lib/AST/ASTImporter.cpp:2131 D2CXX->setDescribedClassTemplate(ToDescribed); +if

[PATCH] D51178: [ASTImporter] Add test for importing anonymous namespaces.

a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: test/Import/cxx-anon-namespace/test.cpp:10 +// This is for the nested anonymous namespace. +// CHECK-NEXT: UsingDirectiveDecl +// CHECK-SAME: '' ---

[PATCH] D51533: [ASTImporter] Merge ExprBits

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Looks good, thanks! Comment at: unittests/AST/ASTImporterTest.cpp:3241 + auto *ToD = Import(FromD, Lang_CXX11); + ASSERT_TRUE(ToD); + auto *ToInitExpr = cast(ToD)->g

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

a_sidorin added a comment. Hi Gabor, The change looks mostly fine but the difference with ASTReader approach disturbs me a bit. Comment at: lib/AST/ASTImporter.cpp:1441 + To->setInit(ToInit); + if (From->isInitKnownICE()) { +EvaluatedStmt *Eval = To->ensureEvaluatedStmt(

[PATCH] D49223: [AST] Check described template at structural equivalence check.

a_sidorin added inline comments. Comment at: lib/AST/ASTStructuralEquivalence.cpp:958 + if (D1->isTemplated() != D2->isTemplated()) +return false; I think we can move the changes for both RecordDecl and FunctionDecl into `Finish()` and use `Decl::getDescr

[PATCH] D49792: [ASTmporter] SourceRange-free function parameter checking for declarations

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

[PATCH] D49796: [ASTImporter] Load external Decls when getting field index.

a_sidorin added a comment. Hi Balázs, The approach is OK but I have some minor comments inline. Comment at: lib/AST/ASTImporter.cpp:2840 - return Index; + assert(false && "Field was not found in its parent context."); + `llvm_unreachable`? ===

[PATCH] D49223: [AST] Check described template at structural equivalence check.

a_sidorin added inline comments. Comment at: lib/AST/ASTStructuralEquivalence.cpp:958 + if (D1->isTemplated() != D2->isTemplated()) +return false; balazske wrote: > a_sidorin wrote: > > I think we can move the changes for both RecordDecl and FunctionDecl i

[PATCH] D42645: New simple Checker for mmap calls

a.sidorin added a comment. Hello David, I have looked into mmap constant definitions in different implementations and found them pretty inconsistent. For example, MMAP_EXEC can be 0x01, 0x04 and I even found 0x00 in some file (https://www.cs.cmu.edu/~dga/crypto/priveth/libethash/mmap.h). There

[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl

a.sidorin created this revision. a.sidorin added reviewers: xazax.hun, szepet, jingham. Herald added a subscriber: rnkovacs. Also minor refactoring in related functions was done. Repository: rC Clang https://reviews.llvm.org/D43012 Files: lib/AST/ASTImporter.cpp test/ASTMerge/var-cpp/Inp

[PATCH] D43074: [Analyzer] Fix a typo about `categories::MemoryError` in `MallocChecker.cpp`

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D43074 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl

a.sidorin marked 2 inline comments as done. a.sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:4296 // Create the declaration that is being templated. - SourceLocation StartLoc = Importer.Import(DTemplated->getLocStart()); - SourceLocation IdLoc = Importer.

[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl

a.sidorin updated this revision to Diff 133626. a.sidorin marked an inline comment as done. a.sidorin added a comment. Fix style issues found on review. Repository: rC Clang https://reviews.llvm.org/D43012 Files: lib/AST/ASTImporter.cpp test/ASTMerge/var-cpp/Inputs/var1.cpp test/ASTMer

[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl

a.sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:2858 + + // Templated declarations should never appear in the enclosing DeclContext. + if (!D->getDescribedVarTemplate()) martong wrote: > In case of class templates, the explicit instantiatio

[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl

This revision was automatically updated to reflect the committed changes. Closed by commit rL325116: [ASTImporter] Fix lexical DC for templated decls; support… (authored by a.sidorin, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D430

[PATCH] D42645: New simple Checker for mmap calls

a.sidorin added a comment. Hello David, There are still some items for improvement. However, if we move this checker into 'alpha' category, as Artem pointed, I think it can be accepted for merge in its current state and improved later. Repository: rC Clang https://reviews.llvm.org/D42645

[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

a.sidorin accepted this revision. a.sidorin added a comment. Thank you! Just some of nits inline. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:394 + +bool ExprEngine::areTemporaryMaterializationsClear( +ProgramStateRef State, const LocationContext *FromLC, ---

[PATCH] D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases.

a.sidorin accepted this revision. a.sidorin added inline comments. Comment at: include/clang/Analysis/ConstructionContext.h:119 + static const ConstructionContext * + finalize(BumpVectorContext &C, const ConstructionContextLayer *TopLayer); + Maybe just `build(

[PATCH] D42645: New simple Checker for mmap calls

a.sidorin accepted this revision. a.sidorin added a comment. Looks good to me as an 'alpha' checker. Thank you David! I'd prefer this patch to be accepted by somebody else as well before committing it. https://reviews.llvm.org/D42645 ___ cfe-commit

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

a.sidorin accepted this revision. a.sidorin added a comment. Hello Daniel & Gabor, Thank you very much for your work. This patch looks good to me but I think such a change should also be approved by maintainers. https://reviews.llvm.org/D30691 ___

[PATCH] D32751: [ASTImporter] Support new kinds of declarations (mostly Using*)

a.sidorin added a comment. Hello Peter, `if (!ToDecl) return nullptr;` is used if original node is always non-null. `if(!ToDecl && FromDecl) return nullptr;` is used if original node can be null. If the imported node is null, the result of import is null as well so such import is OK. `ObjectXY::

[PATCH] D32751: [ASTImporter] Support new kinds of declarations (mostly Using*)

a.sidorin updated this revision to Diff 102679. a.sidorin marked an inline comment as done. a.sidorin added a comment. Herald added a subscriber: kristof.beyls. Add a FIXME. https://reviews.llvm.org/D32751 Files: lib/AST/ASTImporter.cpp test/ASTMerge/namespace/Inputs/namespace1.cpp test/A

[PATCH] D32751: [ASTImporter] Support new kinds of declarations (mostly Using*)

a.sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:2993 + return nullptr; + } + szepet wrote: > nit: As I see these cases typically handled in the way: > > ``` > FrPattern = .; > ToPattern = ..; > if(FrPattern && !ToPattern) > ``` > Just

[PATCH] D34277: [analyzer] Bump default performance thresholds?

a.sidorin added a comment. Hi Artem, Could you tell what code bases did you use to collect your statistics? I'll try to check the patch on our code bases. I think we should be careful about default settings. Maybe we should introduce another UMK_* for deeper analysis instead? https://reviews

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

a.sidorin added a comment. Will anybody object if I commit this change without a test? This bug seems to be pretty obvious but, unfortunately, I'm not familiar with Objective C. https://reviews.llvm.org/D6550 ___ cfe-commits mailing list cfe-commit

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

a.sidorin added a comment. Thank you Sean, I'll try. https://reviews.llvm.org/D6550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34277: [analyzer] Bump default performance thresholds?

a.sidorin accepted this revision. a.sidorin added a comment. Ok, I hope this will work. Anyway, we can always revert this patch in case of any problems. https://reviews.llvm.org/D34277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://

[PATCH] D16403: Add scope information to CFG

a.sidorin added a comment. Hi Matthias, I have posted a comment about review duplication (more than a year ago!) in your review but you haven't answered. So, all this time we were thinking that you do separate non-related work. @dcoughlin As a reviewer of both patches - could you tell us what's

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

a.sidorin updated this revision to Diff 127283. a.sidorin added a comment. Fixed sanity check. Repository: rC Clang https://reviews.llvm.org/D38694 Files: lib/AST/ASTImporter.cpp unittests/AST/ASTImporterTest.cpp Index: unittests/AST/ASTImporterTest.cpp =

[PATCH] D19979: [analyzer] ScopeContext - initial implementation

a.sidorin added a comment. This patch still depends on scope implementation in CFG. There is no final implementation; after initial implementation is done, I'll update the patch. https://reviews.llvm.org/D19979 ___ cfe-commits mailing list cfe-comm

[PATCH] D41151: [analyzer] Adding LoopContext and improve loop modeling

a.sidorin added a comment. This thing is very similar to https://reviews.llvm.org/D19979. Do we really need to create a separate LoopContext or we can reuse ScopeContext instead? https://reviews.llvm.org/D41151 ___ cfe-commits mailing list cfe-comm

[PATCH] D40560: [analyzer] WIP: Get construction into `operator new` running in simple cases.

a.sidorin added a comment. Hi Artem. This patch looks OK, just stylish issues. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112 + // It means that we cannot handle construction into null or garbage pointers. + // Such cosntructors need to be handled by checkers to en

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

a.sidorin updated this revision to Diff 127375. a.sidorin added reviewers: xazax.hun, szepet. a.sidorin added a comment. Herald added a subscriber: rnkovacs. Removed already fixed stuff, added a test for remaining. Repository: rC Clang https://reviews.llvm.org/D6550 Files: lib/AST/ASTImpor

[PATCH] D41077: [analyser] different.CallArgsOrder checker implementation

a.sidorin added a comment. Hi Alexey, This commit strongly needs testing on some real code. I cannot predict the TP rate of this checker now. Regarding implementation, you can find some remarks inline. Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:31 + +cla

[PATCH] D41409: [analyzer] Fix intermediate diagnostics on paths that go through operator new().

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Looks good! Repository: rC Clang https://reviews.llvm.org/D41409 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D41408: [analyzer] NFC: Fix nothrow operator new definition in a test.

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Looks good, just a minor nit. Comment at: test/Analysis/NewDelete-custom.cpp:7 -#if !LEAKS +#if !(LEAKS && !ALLOCATOR_INLINING) // expected-no-diagnostics --

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

a.sidorin created this revision. a.sidorin added reviewers: NoQ, xazax.hun, szepet. Herald added subscribers: cfe-commits, rnkovacs. While running ASTImporterTests, we often forget about Windows MSVC buildbots which enable '-fdelayed-template-parsing' by default. It takes reviewing time to find

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

a.sidorin added a comment. In https://reviews.llvm.org/D41444#960841, @xazax.hun wrote: > Is it possible that this will hide other problems? Wouldn't it be better to > run the tests twice once with this argument and once without it? I don't think so. In fact, without instantiation, we are not

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

a.sidorin added a comment. In case of `-fdelayed-template-parsing`, this code won't be even visible to the Importer. In my opinion, we shouldn't care about code we're not going to import. If we want to import it, we should make it visible and instantiate it. In this case it will not compile. Ho

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

a.sidorin updated this revision to Diff 127748. a.sidorin added a comment. Test both with and without '-fdelayed-template-parsing' in C++ mode. Repository: rC Clang https://reviews.llvm.org/D41444 Files: unittests/AST/ASTImporterTest.cpp Index: unittests/AST/ASTImporterTest.cpp ==

  1   2   3   4   5   >