[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

[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

[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

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

[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

[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

[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/

[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

[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

[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(); -

[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

[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

[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

[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

[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

[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. @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

[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

[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

[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

[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

[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

[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 > >

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

[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

[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

[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

[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/

[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

[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

[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

[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

[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

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

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

[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

[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

[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:

[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

[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

[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] 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/

[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

[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] 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/

[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

[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/

[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

[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

[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 =

[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

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

[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

[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

[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

[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

[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

[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

[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

[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

[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 >

[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] 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:

[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?

[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

[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;

[PATCH] D66667: Debug Info: Support for DW_AT_export_symbols for anonymous structs

shafik updated this revision to Diff 217271. shafik marked an inline comment as done. shafik added a comment. Updating test to be more specific CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7/new/ https://reviews.llvm.org/D7 Files: lib/CodeGen/CGDebugInfo.cpp

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

shafik accepted this revision. shafik added a comment. Other than my two comment this LGTM Comment at: clang/lib/AST/ASTImporter.cpp:3452 << FoundField->getType(); - - return make_error(ImportError::NameConflict); + ConflictingDecls.push_back(FoundField);

[PATCH] D66667: Debug Info: Support for DW_AT_export_symbols for anonymous structs

This revision was automatically updated to reflect the committed changes. Closed by commit rG528f5da6d862: Debug Info: Support for DW_AT_export_symbols for anonymous structs (authored by shafik). Herald added a project: clang. Changed prior to commit:

[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

shafik added inline comments. Comment at: lib/AST/ASTImporter.cpp:949 +return Importer.GetFromTU(Found) == From->getTranslationUnitDecl(); + return From->isInAnonymousNamespace() == Found->isInAnonymousNamespace(); +} I am not sure what case this covers?

[PATCH] D66999: [ASTImporter][NFC] Add comments to code where we cannot use HandleNameConflict

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3454 << FoundField->getType(); - + // This case is not handled with HandleNameConflict, because by the time + // when we reach here, the enclosing class is already imported. If there

[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

shafik added a comment. It is worth noting that: typedef int T; typedef int T; is not valid C99 see godbolt Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64480/new/ https://reviews.llvm.org/D64480

[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

shafik added inline comments. Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:343 +::testing::Values( +std::make_tuple(ExternTypedef, ExternTypedef, ExpectLink), +std::make_tuple(ExternTypedef, AnonTypedef, ExpectNotLink),

[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

shafik added a comment. In D64480#1654354 , @balazske wrote: > In D64480#1653629 , @shafik wrote: > > > It is worth noting that: > > > > typedef int T; > > typedef int T; > > > > > > is not valid C99 see

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

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. I was concerned about how this would affect LLDB but after thinking about it I realized that in the DWARF we will just end up with one `DW_TAG_class_type`. Repository: rG LLVM Github

[PATCH] D66933: [ASTImporter] Propagate errors during import of overridden methods.

shafik accepted this revision. shafik added a comment. LGTM but I agree w/ Gabor's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66933/new/ https://reviews.llvm.org/D66933 ___ cfe-commits

[PATCH] D66667: Debug Info: Support for DW_AT_export_symbols for anonymous structs

shafik updated this revision to Diff 217457. shafik marked an inline comment as done. shafik added a comment. Simplifying test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7/new/ https://reviews.llvm.org/D7 Files: lib/CodeGen/CGDebugInfo.cpp

[PATCH] D66667: Debug Info: Support for DW_AT_export_symbols for anonymous structs

shafik created this revision. shafik added a reviewer: aprantl. shafik added a project: debug-info. shafik added a parent revision: D66605: Debug Info: Support for DW_AT_export_symbols for anonymous structs. This implements the DWARF 5 feature described in:

[PATCH] D67174: Rename of constants in ASTImporterVisibilityTest. NFC.

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

[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

[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)

[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 >

[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

[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] 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:

[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

[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

[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)

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

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

  1   2   3   4   5   6   7   8   9   10   >