[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik added inline comments. Comment at: clang/test/CodeGenCXX/no_auto_return_lambda.cpp:8 + +// CHECK: !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed) +// CHECK-NOT: !DIBasicType(tag: DW_TAG_unspecified_type, name: "auto") aprantl wrote: > This tes

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik updated this revision to Diff 423471. shafik marked 4 inline comments as done. shafik added a comment. - Making test more robust - Adding comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123319/new/ https://reviews.llvm.org/D123319 Files: clang/lib/CodeGen/CGDebugInfo.cp

[PATCH] D123775: [AST] Support template declaration found through using-decl for QualifiedTemplateName.

shafik added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:1004 Name = Context.getQualifiedTemplateName(SS.getScopeRep(), /*HasTemplateKeyword*/ false, +Name);

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik updated this revision to Diff 424300. shafik added a comment. -Replacing CHECK-NEXT with CHECK CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123319/new/ https://reviews.llvm.org/D123319 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/test/CodeGenCXX/no_auto_return_lambda.cpp

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5ff992bca208: [DEBUG-INFO] Change how we handle auto return types for lambda operator() to be… (authored by shafik). Herald added a project: clang.

[PATCH] D119178: Add support for generating debug-info for structured bindings of structs and arrays

shafik created this revision. shafik added reviewers: aprantl, dblaikie. shafik requested review of this revision. Currently we are not emitting debug-info for all cases of structured bindings a C++17 feature which allows us to bind names to subobjects in an initializer. A structured binding is

[PATCH] D119178: Add support for generating debug-info for structured bindings of structs and arrays

shafik added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4667 + + SmallVector Expr; + AppendAddressSpaceXDeref(AddressSpace, Expr); aprantl wrote: > 13 seems to be unnecessarily high. Shouldn't 1 be enough for the expected > single DW_OP_der

[PATCH] D119178: Add support for generating debug-info for structured bindings of structs and arrays

shafik updated this revision to Diff 409056. shafik marked 5 inline comments as done. shafik added a comment. Addressed comments on SmallVector size and fixed test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119178/new/ https://reviews.llvm.org/D119178 Files: clang/lib/CodeGen/CGD

[PATCH] D119178: Add support for generating debug-info for structured bindings of structs and arrays

shafik added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4647 +const bool UsePointerValue) { + assert(CGM.getCodeGenOpts().hasReducedDebugInfo()); + assert(!LexicalBlockStack.empty() && "Region stack mismatch, s

[PATCH] D119178: Add support for generating debug-info for structured bindings of structs and arrays

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf56cb520d855: [DEBUGINFO] [LLDB] Add support for generating debug-info for structured… (authored by shafik). Herald added projects: clang, LLDB. Rep

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik added inline comments. Comment at: clang/test/CodeGenCXX/no_auto_return_lambda.cpp:1 +// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s + erichkeane wrote: > erichkeane wrote: > > So this CodeGen test without a triple is a bit t

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik added a comment. In D123319#3473283 , @dblaikie wrote: > ('scuse the delay) > > Baseline: I'm still not really sure this is the right direction. Is there a > sound argument for why this change is suitable for lambdas, but not for other > types? I

[PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

shafik updated this revision to Diff 427081. shafik removed reviewers: teemperor, jingham, jasonmolenda. shafik added a comment. Herald added a project: All. - Expanded test - applied clang-format CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105564/new/ https://reviews.llvm.org/D105564

[PATCH] D129591: Modify CXXMethodDecl::isMoveAssignmentOperator() to look through type sugar

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane. Herald added a project: All. shafik requested review of this revision. Currently `CXXMethodDecl::isMoveAssignmentOperator()` does not look though type sugar and so if the parameter is a type alias it will not be able

[PATCH] D122768: [Clang][C++20] Support capturing structured bindings in lambdas

shafik added a comment. Herald added a reviewer: NoQ. Thank you for this work. This looks mostly good to me but I am not confident on all the code. I feel like there are some assumptions that maybe could use comments e.g. places where we know it can only be a `VarDecl` e.g. b/c it is an init ca

[PATCH] D129591: Modify CXXMethodDecl::isMoveAssignmentOperator() to look through type sugar

shafik updated this revision to Diff 444744. shafik added a comment. Adding release notes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129591/new/ https://reviews.llvm.org/D129591 Files: clang/docs/ReleaseNotes.rst clang/lib/AST/DeclCXX.cpp clang/test/SemaCXX/cxx0x-defaulted-fu

[PATCH] D129591: Modify CXXMethodDecl::isMoveAssignmentOperator() to look through type sugar

This revision was automatically updated to reflect the committed changes. Closed by commit rG80dec2ecfffe: [Clang] Modify CXXMethodDecl::isMoveAssignmentOperator() to look through type… (authored by shafik). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D128113: Clang: fix AST representation of expanded template arguments.

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1533 + *ReplacedOrErr, ToReplacementTypeOrErr->getCanonicalType(), + T->getPackIndex()); } I think we should have a test in `ASTImporterTest.cpp` to make sure we are importing the

[PATCH] D129973: [clang] Pass FoundDecl to DeclRefExpr creator for operator overloads

shafik added a comment. Thank you for working on this. It looks like there are some pre-commit test failures, did you confirm whether they were related to your change or not? Also you mentioned that this changes the output of `-ast-dump` it might be worth adding a test to capture this behavior

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane. Herald added a project: All. shafik requested review of this revision. DR2338 clarified that it was undefined behavior to set the value outside the

[PATCH] D128276: clang: perform deduction at the right depth on Sema::AddMethodTemplateCandidate

shafik added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:14176 + ParmVarDecl *Param = Method->getParamDecl(i); + if (!Param->hasDefaultArg()) { +IsError = true; Do we have a test for this case? Repository: rG LLVM Github Mon

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik added a comment. Note I took my range generation code from `CGExpr.cpp` function `getRangeForType(...)` I was considering perhaps making the code common, maybe this could be a helper in `EnumDecl`? Does that makes sense? Comment at: clang/lib/AST/ExprConstant.cpp:13512

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik updated this revision to Diff 446045. shafik marked 7 inline comments as done. shafik added a comment. - Added more tests - Added the calculation of the value range to `EnumDecl` so the code can be shared - Modified tests that were failing due to undefined behavior that this change catche

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik added inline comments. Comment at: clang/test/Sema/aarch64-sve-intrinsics/acle_sve_imm.cpp:209 svprfb(pg, const_void_ptr, svprfop(14)); - // expected-error-re@+1 {{argument value {{[0-9]+}} is outside the valid range [0, 13]}} + // expected-error-re@+1 {{must be a co

[PATCH] D130058: [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values

shafik added inline comments. Comment at: clang/include/clang/AST/Decl.h:3834 + void getValueRange(llvm::APInt &Max, llvm::APInt &Min) const; + I should add a documenting comment. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:3

[PATCH] D130210: [SemaCXX] Set promotion type for enum bool to integer type.

shafik added a comment. We seem to have a few tests with enums that bool as a specified type. Maybe this belongs better in one of those under the namespace `GH56560` I am curious that this does not change the test results of `clang/test/CXX/conv/conv.prom/p4.cpp` Repository: rG LLVM Github

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane, tahonermann. Herald added a project: All. shafik requested review of this revision. Currently in `Sema::ActOnEnumBody(...)` when calculating `NumPositiveBits` we miss the case where there is only a single enumerator

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

shafik updated this revision to Diff 446702. shafik added a comment. Herald added a subscriber: Enna1. -Modified UBSan test to cover the empty enum case. I also refactored the test which was pretty clever but hard to work with as is. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130301/

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

shafik added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:18886 + InitVal.getActiveBits() ? InitVal.getActiveBits() : 1; + NumPositiveBits = std::max(NumPositiveBits, ActiveBits); +} else erichkeane wrote: > What about: > > `std::ma

[PATCH] D129973: [clang] Pass FoundDecl to DeclRefExpr creator for operator overloads

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/test/Index/annotate-operator-call-expr.cpp:21 +// CHECK1: Punctuation: "(" [7:6 - 7:7] CallExpr=operator():3:7 +// CHECK1: Punctuation: ")" [7:7 - 7:8]

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

shafik updated this revision to Diff 446997. shafik marked 4 inline comments as done. shafik added a comment. - Addressed style comments - Changed to use initializer_list version of std::max - Added test to cover enum with -1 as sole enumerator value CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

shafik added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Misc/enum.cpp:10 +enum class EBool : bool { a = 1 } e3; +enum EEmpty {}; erichkeane wrote: > can you do a test on: > > `enum ENeg { a = -1 }` > ? > > That should ALSO have only a singl

[PATCH] D128619: [Clang] Implement P0848 (Conditionally Trivial Special Member Functions)

shafik added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:899 + // triviality properties of the class until selecting a destructor and + // computing the eligibility of SMFs. This is because those member + // functions may have constraints that we need to

[PATCH] D130421: [Clang] De-deprecate volatile compound operations

shafik added a comment. LGTM but I will let Aaron accept it. Comment at: clang/docs/ReleaseNotes.rst:523 This feature is available as an extension in all C and C++ language modes. +- Implemented `P2327 De-deprecating volatile compound operations `

[PATCH] D122768: [Clang][C++20] Support capturing structured bindings in lambdas

shafik added inline comments. Comment at: clang/lib/AST/StmtPrinter.cpp:2166 if (Node->isInitCapture(C)) { + VarDecl *D = cast(C->getCapturedVar()); I wonder if it is worth commenting that only a `VarDecl` can be an init capture and therefore we can

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

shafik added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Misc/enum.cpp:27 + return ((int)e1 != -1) & ((int)e2 != -1) & + // CHECK: error: load of value , which is not a valid value for type 'enum EBool' + ((int)e3 != -1) & ((int)e4 == 1) &

[PATCH] D129973: [clang] Pass FoundDecl to DeclRefExpr creator for operator overloads

shafik added a comment. In D129973#3673094 , @SimplyDanny wrote: > Thank you for the review! Before I push the change into main I may add an > entry to `clang/docs/ReleaseNotes.rst`, right? Yes, please add an entry in the release notes. > And do you a

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

shafik updated this revision to Diff 447500. shafik marked 5 inline comments as done. shafik added a comment. - Adding release note - Fixing comments - Removing top level const on local variable - Adjusting test so checks go after the line they are checking CHANGES SINCE LAST ACTION https://re

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an EnumDecl to cover the case of single enumerator with value zero or an empty enum

shafik added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Misc/enum.cpp:27 + return ((int)e1 != -1) & ((int)e2 != -1) & + // CHECK: error: load of value , which is not a valid value for type 'enum EBool' + ((int)e3 != -1) & ((int)e4 == 1) &

[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an EnumDecl to cover the case of single enumerator with value zero or an empty enum

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGaea82d455113: [Clang] Fix how we set the NumPositiveBits on an EnumDecl to cover the case of… (authored by shafik). Herald added projects: clang, San

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

shafik added a comment. > Any update/further details here? David, apologies for not getting back to you sooner. The context is D105564 which I started working on again recently. I was having difficulties finding a solution that also worked for local lambdas.

[PATCH] D148474: [Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision

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

[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: http://dwarfstd.org/ShowIssue.php?issu

[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

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 test/CodeGenCX

[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 test/CodeGenCXX/debug-info-exp

[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: https://reviews.llvm.org/D7?vs=217457&i

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

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

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

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

[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] 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] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

shafik added a comment. Herald added a reviewer: JDevlieghere. We are going to move forward with this approach (after dealing with the multi-dimensional array case) temporarily. We are seeing crash bugs from this from users and we want to fix it while we explore the solution space more. This PR

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

shafik updated this revision to Diff 292607. shafik added a comment. Fix handling of multi-dimensional arrays. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86660/new/ https://reviews.llvm.org/D86660 Files: clang/lib/AST/ASTImporter.cpp lldb/test/API/commands/expression/codegen-cr

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

This revision was automatically updated to reflect the committed changes. Closed by commit rG6807f244fa67: [ASTImporter] Modifying ImportDeclContext(...) to ensure that we also handle… (authored by shafik). Herald added projects: clang, LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

shafik created this revision. shafik added reviewers: martong, teemperor. Herald added a subscriber: rnkovacs. shafik requested review of this revision. When we fixed `ImportDeclContext(...)` in D71378 to make sure we complete each `FieldDecl` of a `RecordDecl`

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

shafik marked 2 inline comments as done. shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759 + QualType FromTy = ArrayFrom->getElementType(); + QualType ToTy = ArrayTo->getElementType(); + + FromRecordDecl = FromTy->getAsRe

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

shafik marked an inline comment as done. shafik added a subscriber: rsmith. shafik added a comment. @martong I have been experimenting w/ how we pass around `ImportDefinitionKind` and pushing it through to more places does not help. I also tried defaulting most locations to `IDK_Everything` to e

[PATCH] D92962: [ASTImporter] Fix import of a typedef that has an attribute

shafik added a comment. LGTM Comment at: clang/unittests/AST/ASTImporterTest.cpp:6098 + FirstDeclMatcher().match(TU, typedefDecl(hasName("X"))); + auto *ToD = Import(FromD, Lang_CXX17); + ASSERT_TRUE(ToD); We can't check the attribute is present? Repos

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

shafik added a comment. So was the bug we were saying there were falsely equivalent or falsely not equivalent? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88665/new/ https://reviews.llvm.org/D88665 __

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

shafik added a comment. In D88665#2308366 , @martong wrote: >> In D88665#2307562 , @shafik wrote: >> >>> So was the bug we were saying there were falsely equivalent or falsely not >>> equivalent? >> >> I think the

[PATCH] D66782: SourceManager: Prefer Optional over MemoryBuffer*

shafik added a comment. `ASTImporter` changes looks good. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66782/new/ https://reviews.llvm.org/D66782 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/

[PATCH] D88780: Allow interfaces to operate on in-memory buffers with no source location info.

shafik added a comment. This looks reasonable, I am guessing we can't test this b/c it would only come up in a cling use case? Was there ever a design document that Hal and JF were asking for? I was reading through the cfe-dev thread and I don't see it. Repository: rG LLVM Github Monorepo

[PATCH] D89319: [ASTImporter] Fix crash caused by unimported type of FromatAttr

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

[PATCH] D89318: [ASTImporter] Fix crash caused by unset AttributeSpellingListIndex

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/ASTImporter.cpp:8109 + FromAttr->getAttributeSpellingListIndex()); + ToAttr->setPackExpansion(FromAttr->isPackExpansion()); + ToAttr->se

[PATCH] D92101: [Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

shafik added a comment. Should we add a test for the `regular function template` case as well? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92101/new/ https://reviews.llvm.org/D92101 ___ cfe-commits mai

[PATCH] D92106: [ASTImporter] Import the default argument of NonTypeTemplateParmDecl

shafik added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:965 +TEST_P(ASTImporterOptionSpecificTestBase, NonTypeTemplateParmDeclDefaultArg) { + Decl *FromTU = getTuDecl("template struct X {};", Lang_CXX03); + auto From = FirstDeclMatcher().match( ---

[PATCH] D92103: [ASTImporter] Import the default argument of TemplateTypeParmDecl

shafik added inline comments. Comment at: lldb/test/API/commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py:25 -deque_type = "std::deque >" size_type = deque_type + "::size_type" Why do the default arguments not show up

[PATCH] D92016: [ASTImporter] Make the Import() return value consistent with the map of imported decls when merging ClassTemplateSpecializationDecls

shafik added a comment. Great catch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92016/new/ https://reviews.llvm.org/D92016 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D92101: [Clang][Sema] Attempt to fix CTAD faulty copy of non-local typedefs

shafik added a comment. In D92101#2428104 , @martong wrote: > In D92101#2423685 , @shafik wrote: > >> Should we add a test for the `regular function template` case as well? > > I think this issue is specific for a `

[PATCH] D92512: ADT: Change AlignedCharArrayUnion to an alias of std::aligned_union_t, NFC

shafik added inline comments. Comment at: clang/include/clang/AST/APValue.h:405 assert(isInt() && "Invalid accessor"); -return *(APSInt*)(char*)Data.buffer; +return *(APSInt *)(char *)&Data; } I notice that in `ASTTypeTraits.h` we use `reinterpret

[PATCH] D92209: [ASTImporter] Support CXXDeductionGuideDecl with local typedef

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:2522 + // Add to the lookupTable because we could not do that in MapImported. + Importer.AddToLookupTable(ToTypedef); + I am not super excited about this solution, I feel like several bugs

[PATCH] D92529: ASTImporter: Migrate to the FileEntryRef overload of SourceManager::createFileID, NFC

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

[PATCH] D92600: [ASTImporter] Add support for importing GenericSelectionExpr AST nodes.

shafik added a comment. `ASTImporter` changes looks good to me. Comment at: clang/lib/AST/ASTImporter.cpp:6525 + + const ASTContext &context = Importer.getToContext(); + if (E->isResultDependent()) { nitpick `context` -> `ToCtx` to be consistent with other co

[PATCH] D92671: Add diagnostic for for-range-declaration being specificed with thread_local

shafik created this revision. shafik added reviewers: rsmith, aaron.ballman. shafik requested review of this revision. Currently we have a diagnostic that catches the other storage class specifies for the range based for loop declaration but we miss the `thread_local` case. This changes adds a d

[PATCH] D92671: Add diagnostic for for-range-declaration being specificed with thread_local

shafik added a comment. The problem with this approach is given: for (static thread_local int a : A()) {} it just issues one diagnostic and it points to `static` but I am not sure what the right approach to handling this case is. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92671/n

[PATCH] D92671: Add diagnostic for for-range-declaration being specificed with thread_local

This revision was automatically updated to reflect the committed changes. Closed by commit rG6333871f8540: Add diagnostic for for-range-declaration being specificed with thread_local (authored by shafik). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D89554: SourceManager: Clarify that FileInfo always has a ContentCache, NFC

shafik added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:1684 bool MyInvalid = false; ComputeLineNumbers(Diag, Content, ContentCacheAlloc, *this, MyInvalid); if (MyInvalid) Is the `const_cast` just for this line? Maybe we can loc

[PATCH] D89914: SourceManager: Make LastLineNoContentCache and ContentCache::SourceLineCache mutable, NFC

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM, this feels cleaner. Comment at: clang/include/clang/Basic/SourceManager.h:721 mutable FileID LastLineNoFileIDQuery; - mutable SrcMgr::ContentCache *LastLineNoConten

[PATCH] D89554: SourceManager: Clarify that FileInfo always has a ContentCache, NFC

shafik accepted this revision. shafik added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89554/new/ https://reviews.llvm.org/D89554 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailma

[PATCH] D90568: [clang] Add [is|set]Nested methods to NamespaceDecl

shafik added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:7866 SourceLocation(), &Context->Idents.get("std"), - /*PrevDecl*/ nullptr); + /*PrevDecl*/ nullptr, false); NS-

[PATCH] D102492: [clang][AST] Add support for BindingDecl to ASTImporter.

shafik added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:873 + MatchVerifier Verifier; + testImport("int a[2] = {1, 2};" + "auto [declToImport, x] = a;", Can we add a larger variety of tests? Given how simple the `BindingDec

[PATCH] D102640: [ASTimporter] Remove decl from lookup only if it has decl context

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. This is a good catch, thank you for fixing this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102640/new/ https://reviews.llvm.org/D102640 ___

[PATCH] D101777: [clang] p1099 1/5: [NFC] Break out BaseUsingDecl from UsingDecl

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:4623 - Expected ToUsingOrErr = import(D->getUsingDecl()); - if (!ToUsingOrErr) -return ToUsingOrErr.takeError(); + Expected ToIntroducerOrErr = import(D->getIntroducer()); + if (!ToIntroducerOrErr)

[PATCH] D101777: [clang] p1099 1/5: [NFC] Break out BaseUsingDecl from UsingDecl

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:4623 - Expected ToUsingOrErr = import(D->getUsingDecl()); - if (!ToUsingOrErr) -return ToUsingOrErr.takeError(); + Expected ToIntroducerOrErr = import(D->getIntroducer()); + if (!ToIntroducerOrErr)

[PATCH] D110810: [clang][ASTImporter] Simplify code of attribute import [NFC].

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:8456 class AttrImporter { - Error Err = Error::success(); + Error &Err; ASTImporter &Importer; I am enthusiastic about reference members if we can somehow avoid them. Can we use a stru

[PATCH] D110927: [analyzer] Access stored value of a constant array through a pointer to another type

shafik added a comment. In D110927#3036647 , @ASDenysPetrov wrote: > In D110927#3036436 , @steakhal > wrote: > >> I'm pretty sure that `int x4 = ((char*)arr)[1];` is supposed to be valid in >> your summary. >>

[PATCH] D96196: [Sema][NFC] Create Sema::BuildImplicitCast

shafik added inline comments. Comment at: clang/lib/Sema/SemaCast.cpp:1060 if (tcr != TC_Success && msg != 0) { if (SrcExpr.isInvalid()) // if conversion failed, don't report another error return; Can we ever have this case w/ the added `if (SrcEx

[PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik created this revision. shafik added reviewers: aaron.ballman, rsmith, teemperor. Herald added a subscriber: arphaman. shafik requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. Currently `TypePrinter` lumps anonymous classes and unname

[PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik added a comment. Note: I am not fixing how we treat anonymous and unnamed enums, I could not find a way to figure out if an enum was anonymous or not. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96807/new/ https://reviews.llvm.org/D96807 __

[PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik updated this revision to Diff 324431. shafik added a comment. - Went with unnamed enums Vs anonymous enums CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96807/new/ https://reviews.llvm.org/D96807 Files: clang/lib/AST/TypePrinter.cpp clang/test/AST/ast-dump-decl-json.c clan

[PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/AST/TypePrinter.cpp:1308 +} else if ((isa(D) && cast(D)->isAnonymousStructOrUnion()) || +isa(D)) { OS << "anonymous"; aaron.ballman wrote: > I think `

[PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

This revision was automatically updated to reflect the committed changes. shafik marked an inline comment as done. Closed by commit rGecb90b55454e: Modify TypePrinter to differentiate between anonymous struct and unnamed struct (authored by shafik). Herald added projects: clang, LLDB. Repository:

[PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik added a comment. Herald added a subscriber: JDevlieghere. Reverted the changes because I missed the clangd test suite and don't know how long it will take to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96807/new/ https://reviews.llvm

[PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik updated this revision to Diff 325100. shafik added a comment. Herald added subscribers: usaxena95, kadircet. Fixing tests that I missed before. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96807/new/ https://reviews.llvm.org/D96807 Files: clang-tools-extra/clangd/unittests/Fi

[PATCH] D103792: [clang][AST] Set correct DeclContext in ASTImporter lookup table for template params.

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:6005 + // Template parameters for a CXXDeductionGuideDecl are re-used by the + // non-deduction-guide-decl template. That template is imported too, and Is this case covered in the tests?

[PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

shafik created this revision. shafik added reviewers: aprantl, teemperor, labath. Herald added a subscriber: arphaman. shafik requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. Currently when we have a member function that has an auto return

<    1   2   3   4   5   6   7   8   9   10   >