[PATCH] D159133: [Sema] Make C++ functional-style cast warn about dropped qualifiers (-Wcast-qual)

shafik added reviewers: aaron.ballman, clang-language-wg. shafik added a comment. Add more reviewers for visibility. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159133/new/ https://reviews.llvm.org/D159133 ___

[PATCH] D159133: [Sema] Make C++ functional-style cast warn about dropped qualifiers (-Wcast-qual)

shafik added a comment. I think this makes sense especially if it matches up w/ gcc but would like to see more feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159133/new/ https://reviews.llvm.org/D159133 __

[PATCH] D159247: [HLSL] Cleanup support for `this` as an l-value

shafik added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:2174 + static QualType getThisType(const FunctionProtoType *FPT, cor3ntin wrote: > Whitespace only change Spurious change. Comment at: clang/lib/Sema/SemaExprMembe

[PATCH] D159345: [Clang] Handle non-ASCII after line splicing

shafik added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:1757 + // If a UTF-8 codepoint appears immediately after an escaped new line, + // CurPtr may point to the splicing \ at the on the preceding line, + // so we need to skip it. I think that is wh

[PATCH] D159345: [Clang] Handle non-ASCII after line splicing

shafik added a comment. I think this looks good but I would like @tahonermann to review this as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159345/new/ https://reviews.llvm.org/D159345 ___ cfe-c

[PATCH] D138091: [Clang] Fix Sema::ClassifyName so that it classifies EnumConstantDecl as NonType when they are brought into scope via using enum

shafik added a comment. Adding some more reviewers to get more eyes who may be familiar with this corner for the code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138091/new/ https://reviews.llvm.org/D138091 ___ cfe-commits mailing list cf

[PATCH] D138603: [Clang] Implement LWG3823 for __is_aggregate

shafik added inline comments. Comment at: clang/test/SemaCXX/type-traits.cpp:556 static_assert(__is_aggregate(EmptyArMB), ""); static_assert(!__is_aggregate(void), ""); static_assert(!__is_aggregate(const volatile void), ""); Should this be `true` now or

[PATCH] D138822: [clang] Add test for CWG36

shafik added inline comments. Comment at: clang/test/CXX/drs/dr0xx.cpp:454 + +namespace dr36 { // dr36: yes +namespace example1 { aaron.ballman wrote: > It took me a while to convince myself, but yes, I agree that Clang seems to > implement the DR. I had to go b

[PATCH] D138194: [clang][Parser][NFC] Simplify ParseParenExprOrCondition

shafik added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:1286 if (getLangOpts().CPlusPlus) { -Cond = ParseCXXCondition(InitStmt, Loc, CK, MissingOK); +Cond = ParseCXXCondition(InitStmt, Loc, CK, false); } else { Nitpick edit for addin

[PATCH] D138091: [Clang] Fix Sema::ClassifyName so that it classifies EnumConstantDecl as NonType when they are brought into scope via using enum

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:1250-1251 bool ADL = UseArgumentDependentLookup(SS, Result, NextToken.is(tok::l_paren)); - if (Result.isSingleResult() && !ADL && !FirstDecl->isCXXClassMember()) +

[PATCH] D138270: [clang][Sema] Skip checking int expressions for overflow in constexpr functions

shafik added a comment. We may not have good code coverage, what about a case like this: constexpr void f() { int arr[10]{}; arr[1024*1024*1024*1204]; } do you still obtain: error: constexpr function never produces a constant expression [-Winvalid-constexpr] constexpr void

[PATCH] D138091: [Clang] Fix Sema::ClassifyName so that it classifies EnumConstantDecl as NonType when they are brought into scope via using enum

shafik updated this revision to Diff 478630. shafik marked an inline comment as done. shafik added a comment. Add release note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138091/new/ https://reviews.llvm.org/D138091 Files: clang/docs/ReleaseNotes.rst clang/lib/Sema/SemaDecl.cpp

[PATCH] D138091: [Clang] Fix Sema::ClassifyName so that it classifies EnumConstantDecl as NonType when they are brought into scope via using enum

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG54be300f7e0b: [Clang] Fix Sema::ClassifyName so that it classifies EnumConstantDecl as… (authored by shafik). Herald added a project: clang. Reposit

[PATCH] D138947: [Clang] Remove invalid assert from Sema::BuildCXXTypeConstructExpr

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane, davide, rsmith. Herald added a project: All. shafik requested review of this revision. Currently `Sema::BuildCXXTypeConstructExpr` asserts that list initialization must mean we have an `InitListExpr` as well. We have

[PATCH] D138947: [Clang] Remove invalid assert from Sema::BuildCXXTypeConstructExpr

shafik added a comment. The windows failure happened on the build before as well so not related to this change: https://buildkite.com/llvm-project/premerge-checks/builds/123890 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138947/new/ https://reviews.llvm.org/D138947 _

[PATCH] D138947: [Clang] Remove invalid assert from Sema::BuildCXXTypeConstructExpr

shafik added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:1464 - (Exprs.size() == 1 && isa(Exprs[0]))) && - "List initialization must have initializer list as expression."); SourceRange FullRange = SourceRange(TyBeginLoc, RParenOrBraceLoc);

[PATCH] D138947: [Clang] Remove invalid assert from Sema::BuildCXXTypeConstructExpr

shafik updated this revision to Diff 479161. shafik added a comment. Bring back assert but w/ the check for `InitListExpr` removed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138947/new/ https://reviews.llvm.org/D138947 Files: clang/lib/Sema/SemaExprCXX.cpp clang/test/SemaCXX/cx

[PATCH] D138947: [Clang] Adjust assert from Sema::BuildCXXTypeConstructExpr

shafik updated this revision to Diff 479332. shafik added a comment. - Update assert wording - Add release note CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138947/new/ https://reviews.llvm.org/D138947 Files: clang/docs/ReleaseNotes.rst clang/lib/Sema/SemaExprCXX.cpp clang/test/

[PATCH] D138947: [Clang] Adjust assert from Sema::BuildCXXTypeConstructExpr

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGef10f81985f6: [Clang] Adjust assert from Sema::BuildCXXTypeConstructExpr (authored by shafik). Herald added a project: clang. Repository: rG LLVM

[PATCH] D135750: [clang][Interp] Track initialization state of local variables

shafik added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:835 IsTemporary = true; Ty = E->getType(); } Do we really want to the type of the expression? If we have a `ValueDecl` don't we want that type? I think they should be

[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

shafik added a comment. This looks like the correct thing to do but I see further down there are also unconditional uses of `Initializer` that should also be guarded with a check. It would be good to fix those as well since we are here. I am guessing the answer is no since this was found using

[PATCH] D137706: [clang][Interp] Implement IntegralToPointer casts

shafik added a comment. I agree with @aaron.ballman I expect some more tests for this. Comment at: clang/lib/AST/Interp/Pointer.h:70 Pointer(Block *B, unsigned BaseAndOffset); + Pointer(unsigned Offset); Pointer(const Pointer &P); Is the only cast we hav

[PATCH] D139172: [clang] Mark CWG554 as N/A

shafik added a comment. @aaron.ballman do you think it is worth it to provide a link to `p1787` as well? I know you can just goto the issue and see that but it feels helpful. I actually missed this at first b/c I usually goto end of the issue to look for the resolution and was confused. Repos

[PATCH] D139173: [clang] Add test for CWG600

shafik added inline comments. Comment at: clang/test/CXX/drs/dr6xx.cpp:18 + sp->f(2); + sp->f(2.2); // expected-error {{is a private member}} +} Maybe add a comment above this saying something like: ``` // access control is applied after overload resolution //

[PATCH] D139173: [clang] Add test for CWG600

shafik added a comment. Thank you for updating the DR statuses! This is much appreciated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139173/new/ https://reviews.llvm.org/D139173 ___ cfe-commits mailin

[PATCH] D139173: [clang] Add test for CWG600

shafik added inline comments. Comment at: clang/test/CXX/drs/dr6xx.cpp:18 + sp->f(2); + sp->f(2.2); // expected-error {{is a private member}} +} Endill wrote: > aaron.ballman wrote: > > Endill wrote: > > > Endill wrote: > > > > shafik wrote: > > > > > Maybe add

[PATCH] D139261: [Clang] Modify sanity check assert in AggExprEmitter::VisitInitListExpr to cover anonymous struct in a union GNU extension

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane. Herald added a project: All. shafik requested review of this revision. `AggExprEmitter::VisitInitListExpr` sanity checks that an empty union is really empty and not a semantic analysis failure. The assert is missing

[PATCH] D139261: [Clang] Modify sanity check assert in AggExprEmitter::VisitInitListExpr to cover anonymous struct in a union GNU extension

shafik added a comment. I realize I am modifying codegen but the test is in sema, I am happy to move the test to somewhere more appropriate if someone has a good suggestion. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139261/new/ https://reviews.llvm.org/D139261

[PATCH] D74639: Add linkage check to ASTNodeImporter::hasSameVisibilityContext and rename to hasSameVisibilityContextAndLinkage

This revision was automatically updated to reflect the committed changes. Closed by commit rGbf3f427ba239: [ASTImporter] Add linkage check to ASTNodeImporter::hasSameVisibilityContext… (authored by shafik). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D74639?

[PATCH] D75048: [ASTImporter] Improved import of AlignedAttr.

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM aside from the comments I made. Comment at: clang/lib/AST/ASTImporter.cpp:7944 + if (auto ToEOrErr = Import(From->getAlignmentExpr())) +To = AlignedAttr::Cr

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

shafik added a comment. A few comments and I would like @teemperor to give this a look as well but it looks good to me. Comment at: clang/lib/AST/ASTImporter.cpp:3281 + // Import the function parameters. + SmallVector Parameters; I am curious, why move thi

[PATCH] D75048: [ASTImporter] Improved import of AlignedAttr.

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:7944 + if (auto ToEOrErr = Import(From->getAlignmentExpr())) +To = AlignedAttr::Create(ToContext, true, *ToEOrErr, ToRange, + FromAttr->getSyntax(), --

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

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

shafik added a comment. In D70524#1767775 , @aprantl wrote: > @shafik Can you speak about whether this feature ("auto" return type in DWARF > v5 section 5.2) would be useful for LLDB? I agree with @dblaikie here, I can't come up with a scenario where th

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

shafik added a comment. Apologies for wacky C++ code that follows but will this also work for the following cases: auto f2() { auto l = []() { struct X{}; return X(); }; return l(); } auto f3() { if ( struct X{} x; true) return X(); else

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

shafik accepted this revision. shafik added a comment. Besides my comments about different scenarios this LGTM Comment at: clang/lib/AST/ASTImporter.cpp:3018 + +bool ASTNodeImporter::hasAutoReturnTypeDeclaredInside(FunctionDecl *D) { + QualType FromTy = D->getType(); -

[PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

shafik created this revision. shafik added reviewers: martong, teemperor. Herald added a subscriber: rnkovacs. This fix was motivated by a crashes in expression parsing during code generation in which we had a `RecordDecl` that had incomplete `FieldDecl`. During code generation when computing th

[PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

shafik added a comment. In D71378#1781616 , @teemperor wrote: > I wonder if we have a way to fix this from with LLDB. Having Clang code that > is only tested in LLDB is always a bit weird. > > Otherwise the idea itself LGTM, thanks for working on this (an

[PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

shafik updated this revision to Diff 234619. shafik marked 3 inline comments as done. shafik added a comment. - Fix typo - Adjust error handing - clang-format test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71378/new/ https://reviews.llvm.org/D71378 Files: clang/lib/AST/ASTImporte

[PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

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

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

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

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

shafik added a comment. LGTM WDYT @teemperor Comment at: clang/lib/AST/ASTImporter.cpp:3638 + /// Number of similar looking friends. + unsigned int TotalCount; + /// Index of the specific FriendDecl. `uint32_t` Is there a reason to not prefer fixed width in

[PATCH] D75922: [ASTImporter] Compare the DC of the underlying type of a FriendDecl

shafik added a subscriber: rsmith. shafik added a comment. I believe that your main example violates basic.scope.class p2 : > A name N used in a class S shall refer to the same declaration in its context > and when re-evaluated in the completed scope

[PATCH] D75560: Make Decl:: setOwningModuleID() public. (NFC)

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

[PATCH] D76385: Allow remapping Clang module include paths

shafik added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2487 + // Return a StringRef to the remapped Path. + auto RemapPath = [&](std::string Path) -> std::string { +Path = remapDIPath(Path); `&` -> `&TheCU` We should try to explicitly

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik added a comment. Also note that blocks are an clang extension and also work in C whereas lambdas are a standard C++ feature and naming although implementation defined mostly falls out of the fact that C++ has member functions and so can be placed inside class as `operator()`. CHANGES S

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik added a comment. In D73282#1855888 , @dblaikie wrote: > I don't think the more targeted fix is a good thing - is the same problem of > not demangling names not there for the other cases that the original fix > addressed? AFAIK no, the other case

[PATCH] D73675: Avoid many std::tie/tuple instantiations in ASTImporter

shafik added a comment. I forgot to mention this earlier but LLDB is a major user of the `ASTImporter` and so in general it is a good idea to run `check-lldb` as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73675/new/ https://reviews.llvm.o

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

This revision was automatically updated to reflect the committed changes. Closed by commit rG428583dd22fd: [DebugInfo] Fix debug-info generation for block invocations so that we set the… (authored by shafik). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:2600 + EnumDecl *FoundDef = FoundEnum->getDefinition(); + if (D->isThisDeclarationADefinition() && FoundDef) +return Importer.MapImported(D, FoundDef); Can you e

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

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

[PATCH] D74639: Add linkage check to ASTNodeImporter::hasSameVisibilityContext and rename to hasSameVisibilityContextAndLinkage

shafik created this revision. shafik added reviewers: martong, teemperor, balazske, a_sidorin. Herald added a subscriber: rnkovacs. This fixed is based on the assert in `LinkageComputer::getLVForDecl(…)` which assumes that all the decls in a redecl chain have the same linkage. This change was tr

[PATCH] D74639: Add linkage check to ASTNodeImporter::hasSameVisibilityContext and rename to hasSameVisibilityContextAndLinkage

shafik added a comment. I would love to have a minimal test case for this but every approach I tried has failed, so I would appreciate if there are ideas here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74639/new/ https://reviews.llvm.org/D74639 __

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik created this revision. shafik added a reviewer: aprantl. clang has an extension for block invocations whose mangled name starts with `___Z`. Currently when generating debug-info for block invo

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3659 // Use llvm function name. -Name = Fn->getName(); +if (Fn->getName().startswith("___Z")) + LinkageName = Fn->getName(); dbl

[PATCH] D72531: Set traversal explicitly where needed in tests

shafik added a comment. I was looking at the changes to `ASTImporterTest.cpp` and it not obvious to me how you determined where it was needed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72531/new/ https://reviews.llvm.org/D72531

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik added a comment. Example as requested, using the same code from `clang/test/CodeGenCXX/debug-info-block-invocation-linkage-name.cpp` the DWARF for block created in `f()` and passed to `g()` as an argument changes from: 0x0052: DW_TAG_subprogram DW_AT_low_pc (0x

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik updated this revision to Diff 241508. shafik marked 4 inline comments as done. shafik added a comment. Address comments by not special casing the `___Z` case, this requires setting both the Name and Linkage name because DWARF requires subroutines to have a `DW_AT_name`. We discovered this

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3659 // Use llvm function name. -Name = Fn->getName(); +if (Fn->getName().startswith("___Z")) + LinkageName = Fn->getName(); dbl

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3659 // Use llvm function name. -Name = Fn->getName(); +if (Fn->getName().startswith("___Z")) + LinkageName = Fn->getName(); dbl

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik updated this revision to Diff 241638. shafik added a comment. Using a more targeted approach. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73282/new/ https://reviews.llvm.org/D73282 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/test/CodeGenCXX/debug-info-block-invocation-l

[PATCH] D73282: Fix debug-info generation for block invocations so that we set the LinkageName instead of the Name

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3659 // Use llvm function name. -Name = Fn->getName(); +if (Fn->getName().startswith("___Z")) + LinkageName = Fn->getName(); dbl

[PATCH] D73675: Avoid many std::tie/tuple instantiations in ASTImporter

shafik added a comment. Thank you for this work, it is awesome! I really like the removal of `importSeq` I feel like the resulting code is more clear. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73675/new/ https://reviews.llvm.org/D73675 ___

[PATCH] D73675: Avoid many std::tie/tuple instantiations in ASTImporter

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1181 + Error Err = Error::success(); + QualType ToElementType = T->getElementType(); + Expr *ToSizeExpr = T->getSizeExpr(); `importChecked`? Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D73675: Avoid many std::tie/tuple instantiations in ASTImporter

shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1168 + Error Err = Error::success(); + QualType ToElementType = T->getElementType(); + Expr *ToSizeExpr = T->getSizeExpr(); Should this group be using `importChecked` as well? Repository

[PATCH] D145408: Fix false positive with unreachable C++ catch handlers

shafik added a comment. Apologies for late review. Comment at: clang/lib/Sema/SemaStmt.cpp:4405 ASTContext &Ctx; - const llvm::DenseMap &TypesToCheck; - const bool CheckAgainstPointer; + const llvm::DenseMap &TypesToCheck; Can we just make `llvm::DenseM

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

shafik created this revision. shafik added reviewers: rsmith, aaron.ballman, erichkeane. Herald added a project: All. shafik requested review of this revision. Prior to P2448R2 we were more aggressive in diagnosing ill-formed `constexpr` functions. Many of these restric

[PATCH] D142490: [Clang] Fix ClassifyImplicitMemberAccess to handle cases where the access in an unevaluated context is not within a CXXRecordDecl or CXXMethodDecl

shafik updated this revision to Diff 505252. shafik marked 2 inline comments as done. shafik added a comment. - Switched to using auto in two if statements - Added Release notes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142490/new/ https://reviews.llvm.org/D142490 Files: clang/do

[PATCH] D142490: [Clang] Fix ClassifyImplicitMemberAccess to handle cases where the access in an unevaluated context is not within a CXXRecordDecl or CXXMethodDecl

shafik added inline comments. Comment at: clang/test/SemaCXX/statements.cpp:60-61 +int i; +int j = ({i;}); // expected-error {{invalid use of non-static data member 'i'}} +// expected-error@-1 {{cannot initialize a member subobject of type 'int' with

[PATCH] D142490: [Clang] Fix ClassifyImplicitMemberAccess to handle cases where the access in an unevaluated context is not within a CXXRecordDecl or CXXMethodDecl

This revision was automatically updated to reflect the committed changes. Closed by commit rGc41be8fc741d: [Clang] Fix ClassifyImplicitMemberAccess to handle cases where the access in an… (authored by shafik). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D145737: PR60985: Fix merging of lambda closure types across modules.

shafik added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:7364 ReadingKindTracker ReadingKind(Read_Decl, *this); + Deserializing D(this); Curious, why do we need this here and below. Comment at: clang/lib/Serializati

[PATCH] D142617: [clang][Interp] Check This pointer without creating InterpFrame

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

[PATCH] D143334: [clang][Interp] Fix diagnosing uninitialized ctor record arrays

shafik added inline comments. Comment at: clang/lib/AST/Interp/Interp.cpp:390 - if (isa(ElemType.getTypePtr())) { + if (ElemType->isRecordType()) { const Record *R = BasePtr.getElemRecord(); aaron.ballman wrote: > The difference between these two is that

[PATCH] D146156: [clang][Lexer] Fix crash/assert clang::HeaderSearch::search_dir_nth

shafik added a comment. Is there a way to test this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146156/new/ https://reviews.llvm.org/D146156 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D146168: [Sema] Stop stripping CV quals from *this captures in lambdas

shafik added a comment. Can you add a summary, I realize folks can goto the release notes edit and see the bug report and then go look it up but at least a cursory summary is a little more reviewer friendly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D146168: [Sema] Stop stripping CV quals from *this captures in lambdas

shafik added a comment. Can we also add the test from the bug report as a regression test, it looks like the existing test are basically covering the same thing but I have seen stranger bugs so it would be good to explicitly cover it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

shafik added a comment. Thank you for the fix. Comment at: clang/lib/Sema/SemaExpr.cpp:17897 +if (SemaRef.FailedImmediateInvocations.contains(E)) + CurrentII->setInt(1); + } else { It is not obvious to me why this is correct, can you pleas

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

shafik added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:1303 +OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch, +SourceLocation(), false /* PartialOrdering */); bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnTy

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

shafik added a comment. I also don't get what is going on here, either a more detailed summary or more inline comments should help. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146178/new/ https://reviews.llvm.org/D146178 ___

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

shafik updated this revision to Diff 505985. shafik marked 6 inline comments as done. shafik added a comment. - Updating diagnostic wording - Adding Reference to changes from p2448r2 to the comments - Modified p3 tests to have an extension pass as well CHANGES SINCE LAST ACTION https://reviews

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

shafik updated this revision to Diff 505986. shafik added a comment. - Added release note - Update CXX status CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146090/new/ https://reviews.llvm.org/D146090 Files: clang/docs/ReleaseNotes.rst clang/include/clang/Basic/DiagnosticGroups.td

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

shafik added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9410-9421 +def ext_incorrect_defaulted_comparison_constexpr : Extension< "defaulted definition of %select{%sub{select_defaulted_comparison_kind}1|" "three-way comparison operator}0 "

[PATCH] D146329: [Clang] Fix defaulted equality operator so that it does not attempt to compare unnamed bit-fields

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane, rsmith. Herald added a project: All. shafik requested review of this revision. If we look at class.bit p2 it tells us that that unnamed bit-fields are not members and

[PATCH] D146329: [Clang] Fix defaulted equality operator so that it does not attempt to compare unnamed bit-fields

shafik added a comment. I would have loved to test the case from https://github.com/llvm/llvm-project/issues/61335 directly but I think in order to do it nicely I need `__builtin_memset` to be usable in a constant expression context. I will add this to my todo list. I am open to other alternati

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

shafik added a comment. Thank you for submitting this fix. Comment at: clang/lib/AST/ExprConstant.cpp:2270 + Info, MTE->getExprLoc(), TempType, *V, Kind, + nullptr, CheckedTemps)) return false; ---

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

shafik added inline comments. Comment at: clang/lib/AST/Interp/Interp.cpp:373 + const FieldDecl *SubObjDecl) { + S.FFDiag(SI, diag::note_constexpr_uninitialized) << SubObjDecl; + S.Note(SubObjDecl->getLocation(), shafik

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

shafik added a comment. Please edit the summary to indicate this fixes: https://github.com/llvm/llvm-project/issues/61441 I am happy someone had more time to dig into this problem after my initial investigation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

shafik added inline comments. Comment at: clang/test/SemaCXX/crash-params.cpp:3 + +template +int foo() { If this test really does not fit into any other test set the please rename the test file `GH61441.cpp` so we know it is a regression test for that github bu

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

shafik added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1714 /// Check whether a function's parameter types are all literal types. If so, /// return true. If not, produce a suitable diagnostic and return false. static bool CheckConstexprParameterTypes(Sema &Se

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

shafik added a comment. In D146426#4206525 , @ilya-biryukov wrote: > In D146426#4206519 , @shafik wrote: > >> Please edit the summary to indicate this fixes: >> https://github.com/llvm/llvm-project/issues/61441

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

shafik added a reviewer: clang-language-wg. shafik added a comment. Adding clang-language-wg for more visibility Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146426/new/ https://reviews.llvm.org/D146426 ___

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

shafik added a comment. As I noted in the bug report not doing `D.setInvalidType();` does fix this bug and seems harmless since the error diagnostic should prevent us from getting to codegen but it is not clear to me if this has other negative impacts. Reading your replies it is not obvious you

[PATCH] D146678: Fix unexpected nullptr in ConceptSpecializationExpr's ArgsAsWritten field

shafik added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:843 Expr *NewIDC = ConceptSpecializationExpr::Create( - C, CSE->getNamedConcept(), CSD, nullptr, CSE->isInstantiationDependent(), - CSE->containsUnexpandedParameterPack()); + C, CSE->getNamed

[PATCH] D147070: Improve requirement clause limitation on non templated function

shafik accepted this revision. shafik added a comment. LGTM Comment at: clang/lib/Sema/SemaDecl.cpp:11877 // member-declarator shall be present only if the declarator declares a // templated function ([dcl.fct]). if (Expr *TRC = NewFD->getTrailingRequiresClause()

[PATCH] D147070: Improve requirement clause limitation on non templated function

shafik added inline comments. Comment at: clang/test/CXX/dcl.decl/dcl.decl.general/p4-20.cpp:27 + +namespace GH61748 { +template Maybe a union case just for completeness: ``` template union U { void f() requires true; }; ``` CHANGES SINCE LAST ACTION ht

[PATCH] D145852: [Clang][AST] Fix __has_unique_object_representations computation for unnamed bitfields.

shafik added inline comments. Comment at: clang/test/SemaCXX/type-traits.cpp:2886-2889 +struct UnnamedEmptyBitfield { + int named; + int : 0; +}; royjacobson wrote: > royjacobson wrote: > > shafik wrote: > > > aaron.ballman wrote: > > > > I think there's one mo

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

shafik added inline comments. Comment at: clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp:204 +#if __cplusplus == 201703L + // cxx17-error@-3 {{non-type template argument refers to subobject '(int *)1'}} +#endif Shouldn't this be an error b/c it is a tempo

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

shafik added inline comments. Comment at: clang/test/CodeGenCXX/template-arguments.cpp:4 + +template CONSTEXPR T id(T v) { return v; } +template auto value = id(V); I don't see any tests covering unions or enums. Comment at: clang/test/SemaTemp

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

shafik added inline comments. Comment at: clang/include/clang/AST/TemplateBase.h:88 +/// so cannot be dependent. +UncommonValue, + erichkeane wrote: > I definitely hate the name here... Just `Value` makes a bit more sense, but > isn't perfectly accurate.

[PATCH] D144866: [clang] Fix aggregate initialization inside lambda constexpr

shafik added a comment. Thank you for this patch, it looks good. Comment at: clang/lib/AST/ExprConstant.cpp:8763 if (isLambdaCallOperator(Info.CurrentCall->Callee)) { - // Ensure we actually have captured 'this'. (an error will have - // been previously reported

<    3   4   5   6   7   8   9   10   11   12   >