[PATCH] D121176: [ASTStructuralEquivalence] Add support for comparing ObjCCategoryDecl.

a_sidorin added a comment. Hi Volodymyr, Unfortunately, I'm not familiar with Objective-C and its frontend but I have a question. Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1989 + ++ParamT1, ++ParamT2) { +if (!IsStructurallyEquivalent(Context, *ParamT1, *

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

a_sidorin accepted this revision. a_sidorin added a comment. Hello Shafik! The patch looks good, I only have a few stylish nits. Comment at: clang/unittests/AST/ASTImporterTest.cpp:5964 + new SourceWithCompletedTagList(CompletedTags); + clang::ASTContext &context = FromTU

[PATCH] D75740: [ASTImporter] Corrected import of repeated friend declarations.

a_sidorin added a comment. Hello Balazs, This look almost good to me except some comments inline. Comment at: clang/lib/AST/ASTImporter.cpp:3635 +static std::tuple +getFriendCountAndPosition(FriendDecl *FD) { It's better to turn the tuple into a named struct

[PATCH] D75732: [ASTImporter] Added visibility check for variable templates.

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

[PATCH] D74554: [ASTImporter] Added visibility check for scoped enums.

a_sidorin accepted this revision. a_sidorin added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:4870 + Decl *ToTU = getToTuDecl( + R"( + enum class E; Maybe it's better to use just non-raw string literals here? ``` Decl *ToTU = g

[PATCH] D74542: [ASTImporter] Prevent the ASTImporter from creating multiple main FileIDs.

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

[PATCH] D71018: [ASTImporter] Improved import of TypeSourceInfo (TypeLoc)

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. The patch looks good. Thanks! I would prefer to see more tests for other TypeLoc cases, but I think they could be added in the following patches. @shafik Shafik, do you have any suggestio

[PATCH] D71020: [ASTImporter] Friend class decl should not be visible in its context

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

[PATCH] D71020: [ASTImporter] Friend class decl should not be visible in its context

a_sidorin added a comment. Hi Gabor, I have an inline comment for the patch. Otherwise, looks good. Comment at: clang/unittests/AST/ASTImporterTest.cpp:252 + QualType Ty = FD->getFriendType()->getType().getCanonicalType(); + if (isa(Ty)) +Ty = cast(Ty)->getNamedType(); --

[PATCH] D71018: [ASTImporter] Improved import of TypeSourceInfo (TypeLoc)

a_sidorin added a comment. Finally, this FIXME is addressed. Thanks! Are you going to add any tests in the following patches? It looks almost good to me, but I have a question inline. Comment at: clang/lib/AST/ASTImporter.cpp:7914 + +#define IMPORT_TYPE_LOC(NAME)

[PATCH] D71020: [ASTImporter] Friend class decl should not be visible in its context

a_sidorin added a comment. Hi Gabor, This patch looks mostly good to me. Thanks! Comment at: clang/lib/AST/ASTImporter.cpp:334 + FromDC->lookup(FromNamed->getDeclName()); + if (std::find(FromLookup.begin(), FromLookup.end(), FromNamed) != + Fr

[PATCH] D70819: [ASTImporter] Support functions with placeholder return types ...

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Let's wait for Shafik's comments before committing this. Comment at: clang/lib/AST/ASTImporter.cpp:48 #include "clang/Basic/Builtins.h" +#include "clang/

[PATCH] D70819: [ASTImporter] Support functions with placeholder return types ...

a_sidorin added a comment. Hello Gabor, That's an interesting case, thank for fixing this! Comment at: clang/lib/AST/ASTImporter.cpp:3008 +// which is equal to the given DC. +bool isAncestorDeclContextOf(DeclContext *DC, Decl *D) { + DeclContext *DCi = D->getDeclContext(); ---

[PATCH] D68634: [ASTImporter] Imported FunctionDecls inherit attributes from existing functions

a_sidorin added a comment. Ouch! Sorry, Gabor -_\\ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68634/new/ https://reviews.llvm.org/D68634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D68634: [ASTImporter] Imported FunctionDecls inherit attributes from existing functions

a_sidorin added a comment. Hello Balasz, In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too. Repository:

[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] D66336: [ASTImporter] Add development internals docs

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

[PATCH] D66538: [AST] AST structural equivalence to work internally with pairs.

a_sidorin accepted this revision. a_sidorin added a comment. Looks good, thank you for addressing the comments! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66538/new/ https://reviews.llvm.org/D66538

[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.

a_sidorin accepted this revision. a_sidorin added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66866/new/ https://reviews.llvm.org/D66866 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D66565: [analyzer] pr43036: Fix support for operator `sizeof...'.

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

[PATCH] D66538: [AST] AST structural equivalence to work internally with pairs.

a_sidorin added a comment. Hello Balasz, I have some comments inline. Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1579 + D2 = D2->getCanonicalDecl(); + std::pair P = std::make_pair(D1, D2); + `std::pair P{D1, D2}`? Comment at: cl

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

a_sidorin added inline comments. Comment at: clang/docs/InternalsManual.rst:1470 +*templated* class (the ``CXXRecordDecl``) of a ``ClassTemplateDecl`` with +``ClassTemplateDecl::getTemplatedDec()``. And we can get back a pointer of the +"described" class template from the *templa

[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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

a_sidorin 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] 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] 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] 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

  1   2   3   4   5   >