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

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Thank you for adding the additional test. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53699/new/ https://reviews.llvm.org/D53699 ___

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

shafik added a comment. Can you add a test? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56581/new/ https://reviews.llvm.org/D56581 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

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

shafik added a comment. Thank you, this looks good but perhaps some refactoring would help improve the change. Comment at: lib/AST/ASTImporter.cpp:3243 + + if (R) { +CXXDestructorDecl *ToDtor = cast(*R); a_sidorin wrote: > It's better to move this code to

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

shafik created this revision. shafik added reviewers: martong, teemperor, balazske, aaron.ballman. Herald added a subscriber: rnkovacs. When importing classes we may add a CXXMethodDecl more than once to a CXXRecordDecl. This patch will fix the cases we currently know about and handle both the c

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

shafik added a comment. @martong the only issue is that I am seeing a regression on `Analysis/ctu-main.cpp` when I run `check-clang`. I am going to look into it but if you have any insights that would be helpful. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56936/new/ https://reviews

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

shafik marked 17 inline comments as done. shafik added a comment. @martong I don't follow the discussion on `VisitParmVarDecl` an what seems like an alternate fix. Can you elaborate? Comment at: lib/AST/ASTImporter.cpp:3042 + if (FoundByLookup) { +if (auto *MD = dyn_cas

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

shafik updated this revision to Diff 183251. shafik added a comment. Addressing comments on naming in the unit test and refactoring of duplicated code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56936/new/ https://reviews.llvm.org/D56936 Files: lib/AST/ASTImporter.cpp unittests

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

shafik added a comment. @martong so I just tested your `lit-tests.patch` and it looks good! Comment at: lib/AST/ASTImporter.cpp:2959 +} + ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) { balazske wrote: > Code formatting is not OK in this fun

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

shafik updated this revision to Diff 183406. shafik marked 3 inline comments as done. shafik added a comment. Changes based on comments - Refactoring ImportFunctionDeclBody to return Error - Refactoring test/Analysis/ctu-main.cpp based on Gabor's patch - Removing merging of function body for late

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

shafik added a comment. @martong Unfortunately this causes a regression on one of the lldb tests `TestDataFormatterLibcxxVector.py` e.g. lldb-dotest -p TestDataFormatterLibcxxVector.py -t -G gmodules --no-multiprocess So this is specific to modules. If you can't reproduce this I can provide

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

shafik added a subscriber: aprantl. shafik added a comment. I don't believe it is directly related to modules but that it just triggers the issue because it may be bringing in more. You can find a description here https://clang.llvm.org/docs/CommandGuide/clang.html#cmdoption-g and @aprantl poin

[PATCH] D57232: [ASTImporter] Check visibility/linkage of functions and variables

shafik added inline comments. Comment at: lib/AST/ASTImporter.cpp:2954 +return Found->hasExternalFormalLinkage(); + else if (Importer.GetFromTU(Found) == From->getTranslationUnitDecl()) { +if (From->isInAnonymousNamespace()) We don't really need an `else

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

shafik updated this revision to Diff 183923. shafik marked 2 inline comments as done. shafik added a comment. Addressing comments on commenting and naming. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56936/new/ https://reviews.llvm.org/D56936 Files: lib/AST/ASTImporter.cpp test/A

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC352436: [ASTImporter] Fix handling of overriden methods during ASTImport (authored by shafik, committed by ). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56936/new/ http

[PATCH] D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way

shafik added inline comments. Comment at: unittests/AST/ASTImporterTest.cpp:468 + template DeclT *Import(DeclT *From, Language Lang) { +return cast_or_null(Import(cast(From), Lang)); Is this being used in this PR? Repository: rC Clang CHANGES SINCE L

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

shafik added a comment. @martong have you had a chance to look at this some more? We ran into another problem that this fixes partially. Can I help in any way? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56581/new/ https://reviews.llvm.org/D56581 __

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

shafik added a comment. I am happy to discuss and review a revised version of this change. 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

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

shafik added a comment. I reverted this commit since it caused a regression in the lldb test suite, specifically TestDataFormatterLibcxxVector.py. See the logs here http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12003/ Please monitor the lldb built bots when modifying ASTImporter.

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

shafik added a comment. @martong Hello, if I look at the specific test that was failing `TestDataFormatterLibcxxVector.py` it only has the following decorator `@add_test_categories(["libc++"])` so that should not prevent it from running on Linux if you are also building libc++. If you do a loca

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

shafik added inline comments. Comment at: include/clang/AST/Expr.h:1407 public: - enum CharacterKind { -Ascii, -Wide, -UTF8, -UTF16, -UTF32 - }; + enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 }; Minor comment, does it make sense

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

shafik added inline comments. Comment at: include/clang/AST/Expr.h:1407 public: - enum CharacterKind { -Ascii, -Wide, -UTF8, -UTF16, -UTF32 - }; + enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 }; riccibruno wrote: > shafik wrote:

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

shafik added a comment. Did you ever resolve the issue of libcxx tests not running https://reviews.llvm.org/D53697 Repository: rC Clang https://reviews.llvm.org/D53702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

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

shafik added subscribers: rsmith, shafik. shafik added a comment. Herald added a reviewer: shafik. Herald added a subscriber: gamesh411. I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all t

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

shafik requested changes to this revision. shafik added a comment. This revision now requires changes to proceed. The rest of the changes look good. Comment at: lib/AST/ExternalASTMerger.cpp:147 ToTag->setHasExternalLexicalStorage(); - ToTag->setMustBuildLookupTable

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

shafik added a comment. Apologies for my late comments. Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:3780 + typedefNameDecl(hasName("U"))); + ASSERT_TRUE(ToD->getUnderlyingType()->isIncompleteType()); + As far as I can tell `S` is complete in `

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

shafik added a comment. In D53751#1311037 , @martong wrote: > > I didn't actually see this comment get addressed other than to say it won't > > be a problem in practice, which I'm not certain I agree with. Was there a > > reason why this got commit befor

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

shafik added a comment. As pointed out in this comment in another review https://reviews.llvm.org/D44100#1311687 We need to make sure we are running the lldb test suite before committing and watching the bots to make sure the commit does not break them. Thank you Repository: rC Clang CHAN

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

shafik added a comment. Apologies, I meant to make the comment in the child PR Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54898/new/ https://reviews.llvm.org/D54898 ___ cfe-commits mailing list cfe-commits@lists.l

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

shafik added a comment. As pointed out in this comment in another review https://reviews.llvm.org/D44100#1311687 We need to make sure we are running the lldb test suite before committing and watching the bots to make sure the commit does not break them. The change is not purely a refactor sinc

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

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM I don't see any LLDB test regressions but I would prefer another reviewer as well. I am going to be modifying this soon to fix some issues w/ handling built-ins. Repository: rC Clang

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

shafik created this revision. shafik added reviewers: martong, a.sidorin, teemperor, aaron.ballman. Herald added subscribers: jdoerfert, rnkovacs. Currently when we see a built-in we try and import the include location. Instead what we do now is find the buffer like we do for the invalid case and

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

shafik updated this revision to Diff 188753. shafik marked 7 inline comments as done. shafik added a comment. Addressed comments on formatting and missing changes to the `Import_New` version of the code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58743/new/ https://reviews.llvm.org/

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

shafik added a comment. In D58743#1413249 , @lebedev.ri wrote: > Is there a test missing here? This origin of this fix is the LLDB expression parsing issue. We were not able to reduce to a test we could put in `ASTImpoterTest.cpp` but we have a LLDB te

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

shafik added a comment. @teemperor ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58743/new/ https://reviews.llvm.org/D58743 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

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

shafik accepted this revision. shafik added a comment. LGTM, thank you ! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58502/new/ https://reviews.llvm.org/D58502 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

This revision was automatically updated to reflect the committed changes. Closed by commit rL355332: [ASTImporter] Handle built-in when importing SourceLocation and FileID (authored by shafik, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

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

shafik accepted this revision. shafik added a comment. LGTM outside of the question I had. Comment at: lib/AST/ASTImporter.cpp:4967 +template static auto getTemplateDefinition(T *D) -> T * { + auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition(); if (!ToTemplatedD

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

shafik added inline comments. Comment at: lib/AST/ASTImporter.cpp:5147 } +} else { // ODR violation. + // FIXME HandleNameConflict ODR violations are ill-formed no diagnostic required. So currently will this fail for cases that clang proper would

[PATCH] D58830: [ASTImporter] Import member expr with explicit template args

shafik accepted this revision. shafik added a comment. LGTM and I don't see any regressions. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58830/new/ https://reviews.llvm.org/D58830 ___ cfe-commits mailing list cfe-c

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

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

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

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

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

shafik created this revision. shafik added reviewers: teemperor, martong, a_sidorin. Herald added a subscriber: rnkovacs. https://reviews.llvm.org/D51633 added error handling to the `ASTNodeImporter::VisitEnumDecl(...)` for the conflicting names case. This could lead to erroneous return of an er

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

shafik added a comment. I was not able to come up with a test that would detect this issue using either `clang-import-test` nor via any of the methods used in `ASTImpoterTest.cpp`. I created a regression test on the lldb side, which should pass once this is committed: https://reviews.llvm.org/

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

shafik added inline comments. Comment at: lib/AST/ASTImporter.cpp:3418 - for (const auto *Attr : D->attrs()) -ToIndirectField->addAttr(Importer.Import(Attr)); Why is this section of code removed? Repository: rC Clang CHANGES SINCE LAST ACTION https:

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

shafik added a comment. In D59665#1439070 , @martong wrote: > Hi Shafik, > > Thank you for looking into this. This is indeed a bug, because whenever a we > encounter an unnamed EnumDecl in the "from" context then we return with a > nameconflict error. >

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

shafik marked an inline comment as done. shafik added a comment. @martong your idea does not work b/c default construction `DeclarationName()` treats it the same as being empty. So `if (!Name)` is still `true`. Comment at: lib/AST/ASTImporter.cpp:2463 if (!ConflictingDecl

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

shafik marked an inline comment as done. shafik added a comment. In D59665#1442826 , @martong wrote: > In D59665#1442328 , @shafik wrote: > > > @martong your idea does not work b/c default construction > > `Declara

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

shafik created this revision. shafik added reviewers: martong, teemperor, friss, a_sidorin. Herald added subscribers: jdoerfert, rnkovacs. We may try and re-import an EnumDecl while trying to complete it in `IsStructuralMatch(...)` specialization for `EnumDecl`. This change mirrors a similar fix

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

shafik added a comment. LLDB regression test that goes with this fix: https://reviews.llvm.org/D59847 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59845/new/ https://reviews.llvm.org/D59845 ___ cfe-commits mailing list cfe-commits@lists.ll

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

shafik updated this revision to Diff 192466. shafik added a comment. Fixes based on comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59845/new/ https://reviews.llvm.org/D59845 Files: lib/AST/ASTImporter.cpp Index: lib/AST/ASTImporter.cpp =

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

shafik marked 2 inline comments as done. shafik added inline comments. Comment at: lib/AST/ASTImporter.cpp:1951 + // something we're trying to import while completin ToEnum + Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum); + JDevlieghere wrote: > ``` > if (D

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

This revision was automatically updated to reflect the committed changes. shafik marked an inline comment as done. Closed by commit rL357100: [ASTImporter] Fix IsStructuralMatch specialization for EnumDecl to prevent re… (authored by shafik, committed by ). Herald added a project: LLVM. Herald add

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

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

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC357940: [ASTImporter] Call to HandleNameConflict in VisitEnumDecl mistakeningly using… (authored by shafik, committed by ). Herald added a project: clang. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

shafik added inline comments. Comment at: clang/trunk/unittests/AST/ASTImporterTest.cpp:4054 +} } aganea wrote: > Fixes > ``` > [2097/2979] Building CXX object > tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/LookupTest.cpp.o > /mnt/f/svn/cla

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

shafik created this revision. shafik added reviewers: martong, teemperor, aprantl, a_sidorin. Herald added a subscriber: rnkovacs. For a `CXXRecordDecl` the `RecordDeclBits` are stored in the `DeclContext`. Currently when we import the definition of a `CXXRecordDecl` via the `ASTImporter` we do

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

shafik updated this revision to Diff 196757. shafik added a comment. Added test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61140/new/ https://reviews.llvm.org/D61140 Files: lib/AST/ASTImporter.cpp test/Import/cxx-record-flags/Inputs/F.cpp test/Import/cxx-record-flags/test.cpp

[PATCH] D57740: [ASTImporter] Import every Decl in lambda record

shafik added a comment. LGTM although I wish we had more lambda tests. Please remember to check the LLDB build bots when landing the patch. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57740/new/ https://reviews.llvm.org/D57740 __

[PATCH] D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. This looks reasonable to me but I don't have strong opinions on refactoring gtest tests. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57322/new/ https://revi

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

shafik added inline comments. Comment at: unittests/AST/StructuralEquivalenceTest.cpp:251 + +TEST_F(StructuralEquivalenceFunctionTest, CtorVsDtor) { + auto t = makeDecls( Curious, is this test just for completeness or is this somehow related to the overloaded o

[PATCH] D57232: [ASTImporter] Check visibility/linkage of functions and variables

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. Herald added a subscriber: jdoerfert. I ran `check-lldb` locally and it looks good. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57232/new/ https://reviews.ll

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

shafik added a comment. This looks reasonable, I will wait for @martong and/or @a_sidorin to review. FYI LLDB is the other big user of ASTImpoter so it is helpful if you can run `check-lldb` especially on MacOS so you can to catch regressions before committing. After committing please make sure

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

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM, I just ran `check-lldb` and I don't see any regressions. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57910/new/ https://reviews.llvm.org/D57910

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

shafik requested changes to this revision. shafik added a comment. This revision now requires changes to proceed. Actually I was mistaken, we can see the difference for `EnumDecl` and `ClassTemplateSpecializationDecl` as well. For `EnumDecl` before: EnumDecl 0x7fd0ae884800 col:6 referenced B

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

shafik added a comment. This LGTM now but I will wait for @teemperor to take a look at it. Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:620 if (predicate(decl->getKind())) { if (log) { I think a comment on the `predica

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

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

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

shafik added a comment. First round of review. Comment at: clang/lib/AST/ASTImporter.cpp:2632 + ExpectedName NameOrErr = Importer.HandleNameConflict( + Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size()); + if (!NameOrErr) `Name

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3859 ToTypeSourceInfo, D->getStorageClass(), /*DefaultArg*/ nullptr)) return ToParm; This should be `DefaultArg` now?

[PATCH] D65573: Add User docs for ASTImporter

shafik added a comment. Thank you for writing this up! I just have a few minor comments. Comment at: clang/docs/LibASTImporter.rst:110 + +Now we create the Importer and do the import: + Maybe helpful to link to the [Matching the Clang AST](https://clang.llvm.o

[PATCH] D66028: [ASTDump] Add is_anonymous to VisitCXXRecordDecl

shafik created this revision. shafik added reviewers: aaron.ballman, aprantl. Adding is_anonymous the ASTDump for `CXXRecordDecl`. This turned out to be useful when debugging some problems with how LLDB creates ASTs from DWARF. https://reviews.llvm.org/D66028 Files: lib/AST/TextNodeDumper.cp

[PATCH] D66028: [ASTDump] Add is_anonymous to VisitCXXRecordDecl

This revision was automatically updated to reflect the committed changes. Closed by commit rL368591: [ASTDump] Add is_anonymous to VisitCXXRecordDecl (authored by shafik, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://revi

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

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. I was hoping to be able reproduce this in LLDB via an expression like this: expr testImportOfDelegateConstructor(10) == 10 but it does not. I am assuming the test ctu test case invokes the i

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

shafik added a comment. Just wanted to see if you were planning on landing this soon, the fix `Name` -> `SearchName` is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests? Repository

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

shafik added a comment. I am not enthusiastic about this solution but I need to think about it some more. We can see that p0624r2 added assignable lambdas: bool f1() { auto x = []{} = {}; auto x2 = x; retur

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

shafik added a comment. Done with first round. Comment at: lib/AST/ASTImporter.cpp:1643 +if (!ImportedOrErr) { + // For RecordDecls, failed import of a field will break the layout of the + // structure. Handle it as an error. What cases are failed

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

shafik added a comment. I ran `check-lldb` and I hit one regression in `TestFormatters.py`, part of what I am seeing is as follows: AssertionError: False is not True : FileCheck'ing result of `expression --show-types -- *(new foo(47))` Error output: error: no matching constructor for init

[PATCH] D62312: [ASTImporter] Added visibility context check for CXXRecordDecl.

shafik added a comment. Minor comments, I am going to run `check-lldb` now. Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:34 }; +struct GetClassPattern { + using DeclTy = CXXRecordDecl; `GetCXXRecordPattern` feels more consistent.

[PATCH] D62312: [ASTImporter] Added visibility context check for CXXRecordDecl.

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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

shafik added a comment. I don't see any regressions but I am a little uncomfortable since there are no tests. So I would feel better if this was split into three parts: Namespaces, Enums and Templates. Are there small test programs that fail due to the missing data? We can add them as regressi

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

shafik added a comment. So an alternative to testing could be matching the AST output from a `clang-import-test` like we did here: https://reviews.llvm.org/D61140 although it unfortunately looks like only only the AST dump of `NamespaceDecl` output the `SourceLocation` you are fixing, see it o

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

shafik added a comment. I think the AST dump for `EnumDecl` and `ClassTemplateSpecializationDecl` should be dumping the missing `SourceLocations` but I am having trouble tracking down where that should be done. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60499

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

shafik added a comment. Sounds good! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53755/new/ https://reviews.llvm.org/D53755 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

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

shafik added inline comments. Comment at: lib/AST/ASTStructuralEquivalence.cpp:302 +/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling +/// conventions bits but must not compare some other bits, e.g. the noreturn +/// bit. This comment is con

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

shafik added inline comments. Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:118 + template + constexpr T X; + )"; Note this is not well-formed b/c it is not initialized, [see godbolt](https://godbolt.org/z/8xvFwh) Repository:

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

shafik added inline comments. Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:118 + template + constexpr T X; + )"; shafik wrote: > Note this is not well-formed b/c it is not initialized, [see > godbolt](https://godbolt.org/z/8xv

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

shafik added inline comments. Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:151 +}; + +template martong wrote: > balazske wrote: > > `FunctionTemplate` and `FunctionTemplateSpec` are missing? > Yes, because `FunctionTemplates` overload with ea

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

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:7842 +// This implementation is inspired by Sema::mergeDeclAttributes. +void ASTNodeImporter::CopyInheritedAttributes(FunctionDecl *Old, + FunctionDecl *New) { ---

[PATCH] D68722: Add an example showing the alternative to nested designators

shafik created this revision. shafik added a reviewer: rsmith. We have an example showing that a nested designators are a c99 extension but not an example showing how to achieve the same thing using a brace-init-list in a designated-initializer-clause. https://reviews.llvm.org/D68722 Files:

[PATCH] D83972: Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

shafik created this revision. shafik added reviewers: teemperor, martong. Herald added a subscriber: rnkovacs. Once we start the definition of an `ObjCInterfaceDecl` we won't attempt to `ImportDeclContext` later on. Unlike `RecordDecl` case which uses `DefinitionCompleter` to force `completeDefi

[PATCH] D83970: [ASTImporter] Refactor ASTImporter to support custom downstream tests

shafik added a comment. LGTM but I want @martong to take a look as well and accept. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83970/new/ https://reviews.llvm.org/D83970 ___ cfe-commits mailing list

[PATCH] D83972: Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

shafik updated this revision to Diff 280291. shafik added a comment. Updating diff since the parent landed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83972/new/ https://reviews.llvm.org/D83972 Files: clang/lib/AST/ASTImporter.cpp lldb/test/API/lang/objc/bitfield_ivars/TestBitfi

[PATCH] D83972: Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

This revision was automatically updated to reflect the committed changes. Closed by commit rG0db2934b0fa9: [ASTImporter] Modify ImportDefiniton for ObjCInterfaceDecl so that we always… (authored by shafik). Herald added projects: clang, LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE L

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

shafik added a comment. I am open to different approaches to this problem, this is opening attempt at fixing it but I may be missing other interactions. AFAICT starting the definition of `ToRecord` the way `CompleteDecl(…)` does when we know `FromRecord` is not defined is just broken for the

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

shafik created this revision. shafik added reviewers: martong, balazske, teemperor, a_sidorin. Herald added a subscriber: rnkovacs. Herald added a reviewer: a.sidorin. shafik added a comment. I am open to different approaches to this problem, this is opening attempt at fixing it but I may be miss

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

shafik updated this revision to Diff 257552. shafik marked 4 inline comments as done. shafik added a comment. Addressing comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 Files: clang/lib/AST/ASTImporter.cpp lldb/test/API/commands/ex

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik created this revision. shafik added reviewers: teemperor, jingham, jasonmolenda, friss. Herald added a subscriber: kristof.beyls. shafik marked an inline comment as done. shafik added a subscriber: rsmith. shafik added inline comments. Comment at: clang/lib/AST/RecordLayou

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik marked an inline comment as done. shafik added a subscriber: rsmith. shafik added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1190 if (UseExternalLayout) { -// FIXME: This appears to be reversed. if (Base->IsVirtual) @rsm

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

shafik accepted this revision. shafik 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/D75740/new/ https://reviews.llvm.org/D75740 ___

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik updated this revision to Diff 275865. shafik added a comment. Adding a second test that is not arm64 specific. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83008/new/ https://reviews.llvm.org/D83008 Files: clang/lib/AST/RecordLayoutBuilder.cpp lldb/test/Shell/Expr/Inputs/la

[PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik added a comment. In D83008#2131776 , @teemperor wrote: > Thanks for tracking this down, this is a really nasty bug... > > The fix itself is obviously fine, but I think I'm out of the loop regarding > the testing strategy. We talked about adding a C

  1   2   3   4   5   6   7   8   9   10   >