[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Artem, This looks good to me. However, it seems to me that the correct solution is to synthesize a shining new function and include it into the redeclaration chain. This requires much

[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

a_sidorin added a comment. > Mmm, what are the pros and cons? This is how we do it in the ASTImporter. It allows us to correctly handle redeclarations with and without bodies - the declarations in the current AST remain unchanged but a new declaration appears. But it can be a kind of a profes

[PATCH] D61140: Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

a_sidorin added a comment. Hello Shafik! The patch itself is fine, but, as other reviewers pointed, tests are appreciated. I suggest to add a test into ASTImporterTests.cpp - you will find several ways to write tests of different complexity here. I think this change can be tested even with the

[PATCH] D61140: Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

a_sidorin accepted this revision. a_sidorin added a comment. Looks good, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61140/new/ https://reviews.llvm.org/D61140 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.

[PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hello Gabor! This looks good to me, but let's wait for LLDB guys to take a look at the patch. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D59692: [ASTImporter] Fix name conflict 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/D59692/new/ https://reviews.llvm.org/D59692 ___

[PATCH] D61424: [ASTImporter] Fix inequivalence of unresolved exception spec

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Looks good! Comment at: clang/lib/AST/ASTImporter.cpp:3107 +// noexcept-unevaluated, while the newly imported function may have an +// evaluated noexcept. }

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. 👍 Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:277 merger.RemoveSources(importer_source); - return ret; + if (ret_or_error) +

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

a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: lib/AST/ASTImporter.cpp:1441 + To->setInit(ToInit); + if (From->isInitKnownICE()) { +EvaluatedStmt *Eval = To->ensureEvaluatedStmt();

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

a_sidorin added a comment. Hi Gabor, To make the review proces faster, you can split the review on separate parts for Stmts, Decls, Types, etc. Otherwise, the review can last for too long. Comment at: lib/AST/ASTImporter.cpp:162 +return llvm::make_error(); +return

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

a_sidorin added a comment. Herald added a subscriber: Szelethus. Hi Balasz, It's going to take some time to review the whole patch. Comment at: lib/AST/ASTImporter.cpp:194 + // FIXME: This should be the final code. + //auto ToOrErr = Importer.Import(From); + //if

[PATCH] D57902: [AST] Fix structural inequivalence of operators

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

[PATCH] D57905: [ASTImporter][ASTImporterSpecificLookup] Add test for different operators

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM! Comment at: unittests/AST/ASTImporterTest.cpp:4869 + + // FromPlus have a different TU, thus its DeclarationName is different too. + Res = LT.lookup(ToTU, FromP

[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU

a_sidorin added a comment. Hi Gabor, Please find my comments inline. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:232 + + // We do not support CTU across languages (C vs C++). if (LangTo.CPlusPlus != LangFrom.CPlusPlus) { The comment change looks strang

[PATCH] D57236: [ASTImporter] Unify redecl chain tests as type parameterized tests

a_sidorin added a comment. Hi Gabor, The refactoring looks great. I have some minor comments inline. Comment at: unittests/AST/ASTImporterTest.cpp:3549 + void TypedTest_ImportDefinitionThenPrototype() { +Decl *FromTU0 = getTuDecl(getDefinition(), Lang_CXX, "input0.cc"); +

[PATCH] D57910: [ASTImporter] Find previous friend function template

a_sidorin accepted this revision. a_sidorin added a comment. Hi Gabor, This patch LGTM with a minor nit. Comment at: unittests/AST/ASTImporterTest.cpp:5109 + // Currently chains of FunctionTemplateDecls are not implemented + //EXPECT_EQ(Imported->getPreviousDecl(), Friend); +

[PATCH] D57236: [ASTImporter] Unify redecl chain tests as type parameterized tests

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks for the changes! The patch looks completely fine to me now. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57236/new/ https://reviews.llvm.org/D572

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

a_sidorin requested changes to this revision. a_sidorin added a comment. This revision now requires changes to proceed. Hi Tom, The change looks reasonable but the tests need some improvements. Comment at: test/ASTMerge/choose-expr/Inputs/choose1.c:1 +#define abs_int(x) __built

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

a_sidorin added a comment. Hi Tom, Thank for the fixes, now the patch looks almost fine. I have a small nit inline, but I think the patch can land after this fix. Comment at: lib/AST/ASTImporter.cpp:6140 +ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) { + auto

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

a_sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:6160 + // condition-dependent. + bool CondIsTrue = false; + if (!E->isConditionDependent()) aaron.ballman wrote: > tmroeder wrote: > > aaron.ballman wrote: > > > a_sidorin wrote: > > > > aaro

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Tom, Thanks for the fixes! The patch looks good to me now. I have only a small nit inline. Comment at: lib/AST/ASTImporter.cpp:6140 +ExpectedStmt ASTNodeImporter::V

[PATCH] D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, I don't see any problems with the patch. Thanks! I think it will be good to get Shafik's approval as well. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.

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

a_sidorin added a comment. Hi Gabor, The patch looks OK overall but I have some comments inline. Comment at: lib/AST/ASTImporter.cpp:4966 // it has any definition in the redecl chain. -static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) { - CXXRecordDecl *ToTemplate

[PATCH] D62556: [analyzer] NFC: CallDescription: Implement describing C library functions.

a_sidorin added a comment. Hi Artem, Looks mostly good, but I have some comments inline. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1064 // e.g. "{a, b}" represent the qualified names, like "a::b". std::vector QualifiedName; unsigned

[PATCH] D62557: [analyzer] Modernize CStringChecker to use CallDescriptions.

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/D62557/new/ https://reviews.llvm.org/D62557 ___ cfe-com

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hello Gabor, This patch looks good to me. Regarding the related patch: can we use getLambdaManglingNumber() for the comparison? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D64241: [ASTImporter] Fix inequivalence of ClassTemplateInstantiations

a_sidorin added a comment. Hi Gabor, This looks fine, but could you please add a test showing how decl shadowing is handled? I.e. if we have Arg in one TU and both Arg and N::Arg in another TU. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64241/ne

[PATCH] D64241: [ASTImporter] Fix inequivalence of ClassTemplateInstantiations

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Looks good! Sorry for the delay :( Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64241/new/ https://reviews.llvm.org/D64241 ___

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

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, Thank you again for working on this patch. I think it can be committed after minor stylish issues are fixed. Comment at: clang/lib/AST/ASTImporter.cpp:1677 +

[PATCH] D65180: [analyzer] VirtualCallChecker: Improve warning messages.

a_sidorin accepted this revision. a_sidorin added a comment. Hi Artem, The patch looks good to me. I prefer a fully qualified name, however, but it is a matter of taste. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65180/new/ https://reviews.llvm.org/D65180

[PATCH] D65203: [ASTImporter] Do not import FunctionTemplateDecl in record twice.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65203/new/ https://reviews.llvm.org/D65203

[PATCH] D65935: [ASTImporter] Import ctor initializers after setting flags.

a_sidorin added a comment. Hello Balazs, Do I understand correctly that it was unset `ToFunction->setLexicalDeclContext(LexicalDC);` that caused lookup problems? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65935/new/ https://reviews.llvm.org/D65

[PATCH] D65999: [ASTImporter] Import additional flags for functions.

a_sidorin added a comment. Hello Balazs, The patch looks good in general. Comment at: clang/unittests/AST/ASTImporterTest.cpp:5239 +TEST_P(ASTImporterOptionSpecificTestBase, ImportOfDefaultImplicitFunctions) { + // Test that import of implicit functions works and the functio

[PATCH] D65269: [ASTImporter] Fix for import of friend class template with definition.

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/D65269/new/ https://reviews.llvm.org/D65269 ___

[PATCH] D65999: [ASTImporter] Import additional flags for functions.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Balazs, The change looks good. I think it can be committed after an unrelated part is removed. Comment at: clang/unittests/AST/ASTImporterTest.cpp:5373 +INSTANTIA

[PATCH] D65573: Add User docs for ASTImporter

a_sidorin added a comment. That's incredible. Thank you! Comment at: cfe/trunk/docs/LibASTImporter.rst:215 +Node *Result = +const_cast(MatchRes[0].template getNodeAs("bindStr")); +assert(Result); We can avoid const_cast if we change the example

[PATCH] D65935: [ASTImporter] Import ctor initializers after setting flags.

a_sidorin added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3293 + // Import Ctor initializers. + if (auto *FromConstructor = dyn_cast(D)) { I suggest to move it closer to the function body import because import of ctor initializers is a part

[PATCH] D66348: [ASTImporter] Do not look up lambda classes

a_sidorin added a comment. Hello Gabor! I think it's a correct solution for the analyzer: usually, we cannot import a lambda until we have to import some enclosing expression - which means that the lambdas are actually not the same. But I'm not so sure about how it can affect the LLDB logic. @s

[PATCH] D66336: [ASTImporter] Add development internals docs

a_sidorin added a comment. Hello Gabor, This doc will become extremely useful for new developers. Thank you for dumping this sacred knowledge! Comment at: clang/docs/InternalsManual.rst:1470 +*templated* class (the ``CXXRecordDecl``) of a ``ClassTemplateDecl`` with +``ClassTem

[PATCH] D65182: [analyzer] Add fix-it hint support.

a_sidorin added a comment. Hi Artem! The patch looks very promising for CSA, thanks for working on this! Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:343 + /// to produce a good fix-it hint for most path-sensitive warnings. + void addFixItHin

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

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62375/new/ https://reviews.llvm.org/D62375 __

[PATCH] D62373: [ASTImporter] Store import errors for Decls

a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/AST/ASTImporterTest.cpp:4697 +DefaultTestValuesForRunOptions, ); + INSTANTIATE_TEST_CASE_P(ParameterizedTests,

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

a_sidorin added a comment. Hello Gabor, Thank you for the explanation. I got the idea of this patch anyway, but it will be definitely useful for anyone digging into the code. Should we make it a comment for ImportPathTy? Comment at: clang/lib/AST/ASTImporter.cpp:8682 + +void

[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Thanks for the explanation! It will be good if someone else takes a look at this patch. Comment at: clang/include/clang/AST/ASTImporterSharedState.h:40 + /// never cle

[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, The patch looks good. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63603/new/ https://reviews.llvm.org/D63603 __

[PATCH] D62329: [ASTImporter] Structural eq: handle DependentScopeDeclRefExpr

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks for the fixes! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62329/new/ https://reviews.llvm.org/D62329 __

[PATCH] D62131: [ASTImporter] Remove NonEquivalentDecls from ASTImporter

a_sidorin added a comment. > The following happened: During the analysis we compared two Decls which > turned out to be inequivalent, so we cached them. Later during the analysis, > however, we added a new node to the redecl chain of one of these Decls which > we previously compared. Then anoth

[PATCH] D62329: [ASTImporter] Structural eq: handle DependentScopeDeclRefExpr

a_sidorin added inline comments. Comment at: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp:173 +DE2->getQualifier()); + } else if (auto CastE1 = dyn_cast(E1)) { +auto *CastE2 = dyn_cast(E2); Hi Gabor, Is there any test fo

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

a_sidorin added a comment. Hello Gabor, This looks mostly good but I have a question inline. Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1182 + if (D1CXX->isLambda()) { +if (!D2CXX->isLambda()) Should we return false if `D1CXX->isLambd

[PATCH] D64073: [ASTImporter] Fix import of lambda in function param

a_sidorin added a comment. Hello Gabor, There is an inline question about tests; other code looks fine. Comment at: clang/lib/AST/ASTImporter.cpp:1713 +// In case of lambdas, the class already has a definition ptr set, but +// the contained decls are not import

[PATCH] D64078: [ASTImporter] Fix structural ineq of lambdas with different sloc

a_sidorin added a comment. Hi Gabor, it is a nice design question if source locations can participate in structural match or not. The comparison tells us that the same code written in different files is not structurally equivalent and I cannot agree with it. They can be not the same, but their

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

a_sidorin added a comment. Hi Gabor, The patch looks good, but it looks to me that it has a relation to https://reviews.llvm.org/D64078 that is kind of questionable to me. Let's delay landing this patch until the fix direction is clear. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D62484: [ASTImporter] Added visibility context check for EnumDecl.

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/D62484/new/ https://reviews.llvm.org/D62484 ___ cfe-com

[PATCH] D60461: [ASTImporter] Import TemplateParameterLists in function templates.

a_sidorin added a comment. Post-LGTM :) Comment at: lib/AST/ASTImporter.cpp:2789 + if (Num == 0) +return Error::success(); + SmallVector ToTPLists(Num); Please add a newline after return. Comment at: lib/AST/ASTImporter.cpp:2796 +el

[PATCH] D61786: [ASTImporter] Separate unittest files

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, I like this change! LGTM, just a few nits. Comment at: clang/unittests/AST/ASTImporterGenericRedeclTest.cpp:20 + +// FIXME put these structs and the tests rel

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

a_sidorin added a comment. Thank you for digging into this! Feel free to ask me if you encounter any problems with the patch. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100

[PATCH] D61815: [CFG] NFC: Modernize test/Analysis/initializers-cfg-output.cpp.

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

[PATCH] D61814: [CFG] NFC: Remove implicit conversion from CFGTerminator to Stmt *, make it a variant class instead.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. The conversion operator indeed looks non-evident. Comment at: clang/include/clang/Analysis/CFG.h:513 public: - CFGTerminator() = default; - CFGTerminator(Stmt *S, bo

[PATCH] D61814: [CFG] NFC: Remove implicit conversion from CFGTerminator to Stmt *, make it a variant class instead.

a_sidorin added inline comments. Comment at: clang/lib/Analysis/ReachableCode.cpp:465 + CFGTerminator T = Block->getTerminator(); + if (T.getKind() == CFGTerminator::StmtBranch) { +const Stmt *S = T.getStmt(); isStmtBranch()? CHANGES SINCE LAST ACTION h

[PATCH] D62064: [ASTImporter] Fix unhandled cases in ASTImporterLookupTable

a_sidorin added a comment. Hi Gabor, This looks fine, but I have a question inline. Comment at: clang/unittests/AST/ASTImporterTest.cpp:4259 static const RecordDecl * getRecordDeclOfFriend(FriendDecl *FD) { QualType Ty = FD->getFriendType()->getType(); + if (auto *Inner =

[PATCH] D62066: [ASTImporter] Enable disabled but passing tests

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Cool! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62066/new/ https://reviews.llvm.org/D62066

[PATCH] D62131: [ASTImporter] Remove NonEquivalentDecls from ASTImporter

a_sidorin added a comment. Hi Gabor, Could you provide an example of an import sequence leading to this behavior? It's hard for me to imagine such a situation. Comment at: clang/test/ASTMerge/struct/test.c:37 // CHECK: struct2.c:53:37: note: field 'f' has type 'float' here -/

[PATCH] D62064: [ASTImporter] Fix unhandled cases in ASTImporterLookupTable

a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/AST/ASTImporterTest.cpp:4259 static const RecordDecl * getRecordDeclOfFriend(FriendDecl *FD) { QualType Ty = FD->getFriendType()->ge

[PATCH] D62329: [ASTImporter] Structural eq: handle DependentScopeDeclRefExpr

a_sidorin added a comment. Hello Gabor, I have a few questions inline. Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:124 + case DeclarationName::CXXConversionFunctionName: +return true; + Should we check the equivalence of getCXXNameType() in such

[PATCH] D62440: [analyzer] NFC: Change evalCall() to provide a CallEvent.

a_sidorin accepted this revision. a_sidorin added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:372 CheckerContext &C) const { - const Func

[PATCH] D62373: [ASTImporter] Store import errors for Decls

a_sidorin added a comment. Hi Gabor, The idea looks fine to me, but I have some questions inline. Comment at: clang/lib/AST/ASTImporter.cpp:5109 } else { // ODR violation. // FIXME HandleNameConflict + return make_error(ImportError::NameConflict); -

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

a_sidorin added a comment. Hi Gabor, Could you please provide any test for the import itself? Comment at: clang/lib/AST/ASTImporter.cpp:8668 + +bool ASTImporter::ImportPathTy::hasCycleAtBack() { + return Aux[Nodes.back()] > 1; const? Repository: rG LLVM Gi

[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

a_sidorin added a comment. Hi Gabor! I haven't find the import sequence examples we try to fix these ways in any of the three patches these change consists of. Could you please provide some (or point if I missed them)? Comment at: clang/include/clang/AST/ASTImporterSharedStat

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

a_sidorin added a comment. Hi Gabor, thank you a lot! I have to say that even having your patch, I still don't understand why the patch fixes the problem (and what the problem is). As I see, the patch handles the cases where some fields or friends have DeclContext different from ToRD. Could yo

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

a_sidorin added subscribers: bruno, aaron.ballman, rsmith. a_sidorin added a comment. Hello Endre. I agree that it doesn't make sense to have 'errors' in AST merging tools, and the changes for ASTImporter part are welcome. However, I don't feel so positive to modules support changes and I don't

[PATCH] D55734: [analyzer] Revise GenericTaintChecker's internal representation

a_sidorin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:156 +bool isDestinationArgument(unsigned ArgNum) const { + return (std::find(DstArgs.begin(), DstArgs.end(), ArgNum) != + DstArgs.end()); llvm::

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

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, Yes, this looks good. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55280/new/ https://reviews.llvm.org/D55280 _

[PATCH] D67490: [Clang][ASTImporter] Added visibility check for FunctionTemplateDecl.

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Looks good! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67490/new/ https://reviews.llvm.org/D67490 __

[PATCH] D45416: [analyzer] ExprEngine: model GCC inline asm rvalue cast outputs

a.sidorin updated this revision to Diff 142226. a.sidorin added a comment. Rewrite the GCCAsmStmt in the CFG. Repository: rC Clang https://reviews.llvm.org/D45416 Files: include/clang/Analysis/CFG.h lib/Analysis/CFG.cpp lib/StaticAnalyzer/Core/ExprEngine.cpp test/Analysis/asm.cpp In

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

a.sidorin marked 2 inline comments as done. a.sidorin added a comment. Thank you Gabor! Comment at: unittests/AST/ASTImporterTest.cpp:356 + ImportAction(StringRef FromFilename, StringRef ToFilename, + const std::string &DeclName) + : FromFilename(FromFilenam

[PATCH] D45417: [analyzer] Don't crash on printing ConcreteInt of size >64 bits

a.sidorin closed this revision. a.sidorin added a comment. Closed with https://reviews.llvm.org/rC330605. Forgot to mention the Differential Revision, sorry. Repository: rC Clang https://reviews.llvm.org/D45417 ___ cfe-commits mailing list cfe-c

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

a.sidorin added a comment. Hi Artem. Cool patch! Maybe we should create an analyzer option for this? So, all users who want webkit-like behaviour will just enable (or disable, don't know what's better) the option and get the feature. I know that introducing a new option is undesirable but it s

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

This revision was automatically updated to reflect the committed changes. a.sidorin marked an inline comment as done. Closed by commit rL330704: [ASTImporter] Allow testing of import sequences; fix import of typedefs for… (authored by a.sidorin, committed by ). Herald added a subscriber: llvm-comm

[PATCH] D46019: [ASTImporter] Fix isa cast assert

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. This LGTM, but could you please add a test? Repository: rC Clang https://reviews.llvm.org/D46019 ___ cfe-commits mailing list cfe-commit

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

a.sidorin added a comment. Mostly LG. Comment at: lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:75 auto report = llvm::make_unique(*BT, os.str(), N); + report->addVisitor(std::move(Visitor)); report->addRange(SizeE->getSourceRange()); In this patch, som

[PATCH] D46019: [ASTImporter] Fix isa cast assert

a.sidorin added a comment. > `E->getFoundDecl().getDecl()` can be null when a member expression does not > involve lookup. (Note, it may involve a lookup in case of a using directive > which refers to a member function in a base class template.) Yes, a pretty weird example. Unfortunately, they

[PATCH] D45416: [analyzer] ExprEngine: model GCC inline asm rvalue cast outputs

a.sidorin updated this revision to Diff 143959. a.sidorin added a comment. Add a test for CFG dump; replace static_cast with an initialization. No test failures on check-all were observed. Repository: rC Clang https://reviews.llvm.org/D45416 Files: include/clang/Analysis/CFG.h lib/Analys

[PATCH] D46019: [ASTImporter] Fix isa cast assert

a.sidorin added a comment. Confirming LGTM, no objections. Thank you! Repository: rC Clang https://reviews.llvm.org/D46019 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

a.sidorin added inline comments. Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:72 + const FieldDecl *getEndOfChain() const { return Chain.back()->getDecl(); } + template void toString(SmallString &Buf) const; + friend struct FieldChainInfoComparat

[PATCH] D38845: [ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr

a.sidorin requested changes to this revision. a.sidorin added inline comments. This revision now requires changes to proceed. Comment at: unittests/AST/ASTImporterTest.cpp:1569 +FirstDeclMatcher().match(*TB, Pattern); +if (!FromDSDRE) + return; s

[PATCH] D46115: [ASTImporter] properly import SrcLoc of Attr

a.sidorin added a comment. Hi all. I'm already here, invited by the Herald - just had no time to take a look (will change the script to add me as a reviewer). But thank you anyway! :) In the initial implementation (https://raw.githubusercontent.com/haoNoQ/clang/summary-ipa-draft/lib/AST/ASTImpo

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

a.sidorin added a comment. Hi Henry. I had a quick look at the patch, here are some remarks. Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2092 + // 'invalidateRegions()', which will remove the pair + // in CStringLength map. So calls 'InvalidateBuffer()

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

a.sidorin updated this revision to Diff 144998. a.sidorin retitled this revision from "[analyzer] ExprEngine: model GCC inline asm rvalue cast outputs" to "[AST, analyzer] Transform rvalue cast outputs to lvalues (fheinous-gnu-extensions)". a.sidorin edited the summary of this revision. a.sidorin

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

a.sidorin added a comment. Hello Gabor, Thank you for the patch. It looks mostly good, but I have a question: should we add this new declaration to the redeclaration chain like we do it for RecordDecls? Repository: rC Clang https://reviews.llvm.org/D46353 ___

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

a.sidorin added a comment. Hi Rafael, Could you please show the AST we get with `getDecomposedExpansionLoc()`? This change can be an item for a separate patch. https://reviews.llvm.org/D26054 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D38845: [ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr

a.sidorin added a comment. Hello Peter! This looks almost OK, just some minor formatting issues. Comment at: unittests/AST/ASTImporterTest.cpp:1509 + MatchVerifier Verifier; + testImport("template struct S {static T foo;};" + "template void declToImport() {"

[PATCH] D26054: Use `getFileLoc()` instead of `getSpellingLoc()` in the ASTImporter

a.sidorin added a comment. I see,t hank you. Please feel free to submit a patch - it seems like you already have a nice test case that shows the difference between two import options. https://reviews.llvm.org/D26054 ___ cfe-commits mailing list cf

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

a.sidorin created this revision. a.sidorin added reviewers: xazax.hun, martong, szepet, jingham. Herald added subscribers: cfe-commits, rnkovacs. `buildASTFromCodeWithArgs()` accepts `llvm::Twine` as `Code` argument. However, if the argument is not a C string or std::string, the argument is being

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

a.sidorin added subscribers: pcc, klimek. a.sidorin added a comment. Hi Gabor, > Can't we have the same problem with FileName? As I can see, no. FileName is copied into std::string while building compilation arguments. > Perhaps an other alternative would be to make the members real strings.

[PATCH] D38845: [ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr

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

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

a.sidorin added a comment. Hi Rafael! I like the change. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:276 + ASTImporter &Importer = getOrCreateASTImporter(D->getASTContext()); + auto *ToDecl = cast(Importer.Import(const_cast(D))); + assert(HasDefinition(ToDecl)); ---

[PATCH] D46115: [ASTImporter] properly import SrcLoc of Attr

a.sidorin added a comment. Hi Rafael! Please find my comments inline. Comment at: lib/AST/ASTImporter.cpp:2650 + for (const auto *A : D->attrs()) +ToIndirectField->addAttr(Importer.Import(const_cast(A))); Could we just remove 'const' qualifier from `A` t

[PATCH] D46115: [ASTImporter] properly import SrcLoc of Attr

a.sidorin added a comment. Sorry, two more nits. Comment at: include/clang/AST/ASTImporter.h:137 +/// +/// \returns the equivalent attribute in the "to" context, or NULL if an +/// error occurred. nullptr Comment at: lib/AST/ASTIm

[PATCH] D46115: [ASTImporter] properly import SrcLoc of Attr

a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. Looks good! Comment at: lib/AST/ASTImporter.cpp:2650 + for (const auto *A : D->attrs()) +ToIndirectField->addAttr(Importer.Import(const_cast(A))); --

[PATCH] D46115: [ASTImporter] properly import SrcLoc of Attr

This revision was automatically updated to reflect the committed changes. Closed by commit rL331762: [ASTImporter] Properly import SourceLocations of Attrs (authored by a.sidorin, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D46115?v

<    1   2   3   4   5   >