[PATCH] D86993: Document Clang's expectations of the C standard library.

rsmith added a comment. In D86993#4655474 , @RalfJung wrote: > With everything moving to github PRs, what are the next steps for this patch? I'm unlikely to have cycles to work on this in the near future. If someone else wants to take over here, you woul

[PATCH] D157879: [clang] Report missing designated initializers in C++

rsmith added a comment. This change has introduced a false positive for anonymous union members: struct A { int m; union { int n = 0; }; }; A a = A{.m = 0}; now produces a false positive warning saying that the anonymous union member in `A` is uninitialized. Repository:

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

rsmith added a comment. I've been pondering what I'd want from a warning here. I think generally I would like to warn if there are two plausible interpretations of the token sequence -- that is, if giving the `?` different precedence could plausibly lead to a different valid program. I think co

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. In D147655#4650666 , @rjmccall wrote: > Yeah, the more I think about this, the more I think that while (1) Apple > should upstream its use of an older default, regardless (2) the existence of > any targets at all with an older de

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. In D147655#4649864 , @dyung wrote: > Hi @rsmith, we have an internal test where your change seems to have changed > the mangling in C++17 mode and wanted to check if that was intentional. [...] > Are these changes in non-C++20 m

[PATCH] D153701: [Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

rsmith added a comment. The changes in `SemaInit.cpp` don't look correct to me. Trying to recurse through the initializer and find every nested temporary is highly error-prone; I can't tell you which expression nodes you forgot to recurse through, but I'm sure there are some. I think the appro

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. Ping. Are there any further concerns here? (This obviously needs to be merged with trunk, and the `-fclang-abi-compat=` checks and release notes need to be updated to match the latest release version.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D158296: [clang] Diagnose overly complex Record in __builtin_dump_struct

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:732-733 +int RDKind = RD->isClass() ? 0 : (RD->isStruct() ? 1 : 2); +S.Diag(PtrArg->getBeginLoc(), diag::err_builtin_dump_struct_too_complex) +<< RDKind << RD->getName(); +return ExprErr

[PATCH] D158296: [clang] Diagnose overly complex Record in __builtin_dump_struct

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:732-733 +int RDKind = RD->isClass() ? 0 : (RD->isStruct() ? 1 : 2); +S.Diag(PtrArg->getBeginLoc(), diag::err_builtin_dump_struct_too_complex) +<< RDKind << RD->getName(); +return ExprErr

[PATCH] D158967: [clang][clangd] Ensure the stack bottom before building AST

rsmith added a comment. In D158967#4621361 , @zyounan wrote: >> Currently the code in CompilerInstance::ExecuteAction seems to acknowledge >> that there should be a fallback. I'm suggesting to move this fallback down >> to a function that actually runs

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith added a comment. In D155387#4579866 , @ilya-biryukov wrote: > I believe this should compile as according to (over.match.oper)p4 > : > >> A non-template function or function template F na

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

rsmith added a comment. The prior behavior of Clang is correct. A search of a class scope is ill-formed if it finds an ambiguous result, see http://eel.is/c++draft/basic.lookup#class.member.lookup-6.sentence-2 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D158372: [Clang] Treat invalid UDL as two tokens

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:97 ^ -- Implemented `CWG1473 `_ which allows spaces after ``operator""``. - Clang used to err on the lack of space when the literal suffix identifie

[PATCH] D154559: [clang] Fix constant evaluation about static member function

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. Clang was correct here until fairly recently; P2280 (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2280r4.html) changed the language rul

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:14254 int SectionFlags = ASTContext::PSF_Read; -if (var->getType().isConstQualified()) { - if (HasConstInit) efriedma wrote: > rnk wrote: > > I think this is not compatible with M

[PATCH] D156806: [Modules] Add test for merging of template member parent

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Awesome, thanks! It makes perfect sense that rG61c7a9140b would fix this. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This looks good to me. I think we can further refine the handling of duplicate diagnostics but I don't think that needs to be done as part of this bugfix. Comment at: clang/

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

rsmith added a comment. LGTM, though please wait a day or two for any more comments from @gribozavr2 since he's looked at this more closely than I have. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155895/new/ https://reviews.llvm.org/D155895 _

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10339-10343 + if (RT->getDecl()->isAnonymousStructOrUnion()) { +FieldsToCheck.append(RT->getDecl()->field_begin(), + RT->getDecl()->field_end()); +continue; +

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith added a comment. In D154658#4533476 , @MatzeB wrote: > This change results in some of our builds (distributed Thin-LTO in case that > matters) to fail with missing symbols. At a first glance this seems to emit > VTables in some files where it did

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

This revision was automatically updated to reflect the committed changes. Closed by commit rG9d525bf94b25: Optimize emission of `dynamic_cast` to final classes. (authored by rsmith). Changed prior to commit: https://reviews.llvm.org/D154658?vs=542693&id=543133#toc Repository: rG LLVM Github

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

rsmith added a comment. In D154324#4524235 , @v.g.vassilev wrote: > @rsmith, thanks for the suggestions! Could you go over > `ODRHash::AddTemplateName` suggest how to fix it to address > https://reviews.llvm.org/D153003 and https://reviews.llvm.org/D41

[PATCH] D156000: Track the RequestingModule in the HeaderSearch LookupFile cache.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7c5e4efb099e: Track the RequestingModule in the HeaderSearch LookupFile cache. (authored by rsmith). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914 + // [P2718R0] Lifetime extension in range-based for loops. + // + // 6.7.7 [class.temporary] p5: + // There are four contexts in which temporaries are destroyed at a different + // point than

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

rsmith added a comment. I've done a pass through this file looking for places where we incorrectly add to the ODR hash a type that was written within some other entity than the one that we're ODR hashing, that could validly be spelled differently in different declarations of that other entity.

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:15191 OverloadCandidateSet::iterator Best; switch (CandidateSet.BestViableFunction(*this, OpLoc, Best)) { case OR_Success: shafik wrote: > @rsmith if `R.isAmbiguous()` should we ev

[PATCH] D156000: Track the RequestingModule in the HeaderSearch LookupFile cache.

rsmith created this revision. rsmith added a reviewer: jansvoboda11. Herald added a project: All. rsmith requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Different requesting modules can have different lookup results, so don't cache results

[PATCH] D132779: Enforce module decl-use restrictions and private header restrictions in textual headers

rsmith added a comment. In D132779#4523783 , @jansvoboda11 wrote: > Hi @rsmith, this commit makes it possible for `HeaderInfo::LookupFile()` to > be called with different `RequestingModule` within single `CompilerInstance`. > This is problematic, since

[PATCH] D154324: [C++20] [Modules] [ODRHash] Use CanonicalType for base classes

rsmith added a comment. OK, I see. The problem is that the canonical version of the type can be spelled in different ways in different translation units, due to us treating some expressions as being equivalent despite them not being the same under the ODR. For example, we consider these functio

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

rsmith added a comment. This looks correct to me, but it's still a little subtle. Perhaps it'd be clearer to map the method to an integer (0 for copy assignment, 1 for move assignment, 2 for destructor, 3 for equality comparison), and then order them by that integer? That'd be more obviously a

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith added a comment. In D154658#4516554 , @rjmccall wrote: > LGTM, except, should we have a way to turn this optimization off specifically? Sure. I suppose even for an internal linkage vtable there could be a reason to want to turn this off (eg, if s

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith updated this revision to Diff 542693. rsmith added a comment. Herald added a subscriber: MaskRay. - Add option to disable vptr-based dynamic_cast optimization. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154658/new/ https://reviews.llvm.or

[PATCH] D155857: [clang] fix nonnull warnings during build

rsmith accepted this revision as: rsmith. rsmith added a comment. This revision is now accepted and ready to land. Seems reasonable to me. Given that this affects all of LLVM I'd like to wait a day or so to see if anyone has concerns. Comment at: llvm/cmake/modules/HandleLLVMO

[PATCH] D155857: [clang] fix nonnull warnings during build

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. This branch doesn't look necessary or correct to me. We should never call this function on a lazy pointer in offset mode unless we have an external source that produced the offset, a

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

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

[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9591-9594 + bool IsMSConstexpr = Info.CurrentCall->CanEvalMSConstexpr && + OperatorNew->hasAttr(); if (OperatorNew->isReservedGlobalPlacementOperator() && + (Info.CurrentCall->

[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9591-9594 + bool IsMSConstexpr = Info.CurrentCall->CanEvalMSConstexpr && + OperatorNew->hasAttr(); if (OperatorNew->isReservedGlobalPlacementOperator() && + (Info.CurrentCall->

[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:5615-5627 +if (canEvalMSConstexpr || isMSConstexpr) { + // Diagnose invalid usage of [[msvc::constexpr]] function + bool isConstructor = isa(Definition); + if (canEvalMSConstexpr) { // !is

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith added inline comments. Comment at: clang/include/clang/Sema/Lookup.h:228-229 Other.Paths = nullptr; -Other.Diagnose = false; +Other.DiagnoseAccess = false; +Other.DiagnoseAmbiguous = false; return *this; cor3ntin wrote: > rsmith wrote

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith added inline comments. Comment at: clang/include/clang/Sema/Lookup.h:228-229 Other.Paths = nullptr; -Other.Diagnose = false; +Other.DiagnoseAccess = false; +Other.DiagnoseAmbiguous = false; return *this; cor3ntin wrote: > Does anythin

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith added a comment. In D154658#4490468 , @rjmccall wrote: > Oh, this does matter on platforms using pointer authentication, because each > vptr field is signed with a constant discriminator derived from the name of > the class that introduced it. O

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith updated this revision to Diff 539776. rsmith marked an inline comment as done. rsmith added a comment. Herald added a reviewer: jdoerfert. Herald added subscribers: jplehr, sstefan1. - Avoid emitting the type_info when detecting whether it would be null. - Bring back class type in GetVTable

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith updated this revision to Diff 538283. rsmith added a comment. - Mark gep as inbounds. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154658/new/ https://reviews.llvm.org/D154658 Files: clang/lib/AST/ExprCXX.cpp clang/lib/CodeGen/CGCXXABI

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith marked an inline comment as done. rsmith added a comment. In D154658#4481225 , @rjmccall wrote: > In D154658#4479213 , @rsmith wrote: > >> I think (hope?) we should be able to apply this to a much larger se

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith updated this revision to Diff 538281. rsmith added a comment. - Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154658/new/ https://reviews.llvm.org/D154658 Files: clang/lib/AST/ExprCXX.cpp clang/lib/CodeGen/CGCXX

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith added a comment. In D154658#4479170 , @rjmccall wrote: > I don't think it's an intended guarantee of the Itanium ABI that the v-table > will be unique, and v-tables are frequently not unique in the presence of > shared libraries. https://itanium

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rsmith created this revision. rsmith added a reviewer: rjmccall. Herald added subscribers: nlopes, mgrang. Herald added a project: All. rsmith requested review of this revision. Herald added subscribers: cfe-commits, wangpc. Herald added a project: clang. - When the destination is a final class ty

[PATCH] D153701: [Clang] Implement P2718R0 "Lifetime extension in range-based for loops"

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914 + // [P2718R0] Lifetime extension in range-based for loops. + // + // 6.7.7 [class.temporary] p5: + // There are four contexts in which temporaries are destroyed at a different + // point than

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

rsmith added a comment. Making the `return ESR_Failed;` unconditional looks to be the correct change here. We can't continue evaluation past that point because we don't know what would be executed next. Unconditionally returning `ESR_Failed` in that situation is what the other similar paths thr

[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Wow, what an amazing bug :) Comment at: clang/lib/CodeGen/CGDecl.cpp:2459 + if (!isa(Arg.getAnyValue())) +Arg.getAnyValue()->setName(D.getName()); Is

[PATCH] D153375: [Clang] Fix incorrect use of direct initialization with copy initialization

rsmith requested changes to this revision. rsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p3.cpp:293 + e = {E::E1}; + e = {0}; // expected-error {{cannot initialize a value of type 'E' with

[PATCH] D153375: [Clang] Fix incorrect use of direct initialization with copy initialization

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15441-15443 // C++11 5.17p9: // The meaning of x = {v} [...] is that of x = T(v) [...]. The meaning // of x = {} is x = T(). The code change doesn't match this comment, which

[PATCH] D153962: [clang] Do not discard cleanups while processing immediate invocation

rsmith added a comment. LGTM, thank you for doing this! Comment at: clang/lib/Sema/SemaExpr.cpp:18201 +// Hence, in correct code any cleanup objects created inside current +// evaluation context must be outside the immediate invocation. +E = ExprWithCleanups::Create

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

rsmith added a comment. In D153003#4458323 , @ChuanqiXu wrote: > In D153003#4456595 , @rsmith wrote: > >> How to we even get into the ODR hasher here? I thought we only applied it to >> function and class definit

[PATCH] D153003: [ODRHash] Fix ODR hashing of template names

rsmith added a comment. I think the behavior change for the testcase here is correct, though I'm not sure that the patch is getting that behaviour change in the right way. Per [temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4), > Two template-ids are the same if [...] their corresponding te

[PATCH] D153294: [clang] Do not create ExprWithCleanups while checking immediate invocation

rsmith added a comment. A constant expression (including the immediate invocations generated for `consteval` functions) is a full-expression, so destructors should be run at the end of evaluating it, not as part of the enclosing expression. That's presumably why the code is calling `MaybeCreate

[PATCH] D142388: [clang] Add builtin_nondeterministic_value

rsmith added a comment. In D142388#4414375 , @aaron.ballman wrote: > In D142388#4412521 , @rsmith wrote: > >> I have a concern with the name of this builtin. People are going to assume >> it produces a nondeterm

[PATCH] D142388: [clang] Add builtin_nondeterministic_value

rsmith added a comment. I have a concern with the name of this builtin. People are going to assume it produces a nondeterministic value, and use it for seeding random number generators and similar, and will be surprised when the value produced is actually deterministic, and, worse, might leak i

[PATCH] D148700: [clang] Add support for “regular” keyword attributes

rsmith added a comment. In D148700#4401353 , @jyknight wrote: > Yes, standard attributes aren't supposed to be used for things which affect > the type system (although, we certainly have many, already, which do, since > we expose most GCC-syntax attribu

[PATCH] D151952: [clang] adds `__type_pack_index` so we can get a type's parameter pack index

rsmith added a comment. Taking a step back for a moment: what is the intended use case for this? My concern is that most of the time you're going to want to make O(N) such queries into a pack of N elements, resulting in O(N^2) compile time, much like we regrettably get from uses of `__type_pack

[PATCH] D134334: [Clang] Fix crash in isCXXDeclarationSpecifier when attempting to annotate template name

rsmith added inline comments. Comment at: clang/lib/Parse/ParseTentative.cpp:1553-1554 return TPResult::Error; - if (Tok.isNot(tok::identifier)) + if (NextToken().isNot(tok::identifier)) break; } ---

[PATCH] D134334: [Clang] Fix crash in isCXXDeclarationSpecifier when attempting to annotate template name

rsmith requested changes to this revision. rsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Parse/ParseTentative.cpp:1553-1554 return TPResult::Error; - if (Tok.isNot(tok::identifier)) +

[PATCH] D151553: [clang] Fix consteval operators in template contexts

rsmith added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:11940-11943 - ExprResult Callee = getDerived().TransformExpr(E->getCallee()); - if (Callee.isInvalid()) -return ExprError(); - Fznamznon wrote: > cor3ntin wrote: > > I don't understand

[PATCH] D150212: [clang][Sema] Improve diagnostics for auto return type

rsmith accepted this revision. rsmith added a comment. LGTM too. Comment at: clang/test/SemaCXX/auto-type-from-cxx.cpp:31-43 +class Abstract{ + public: +void fun(); +virtual void vfun()=0; +void call(){getCaller()(*this);} // expected-note {{in instantiation of fu

[PATCH] D150212: [clang][Sema] Improve diagnostics for auto return type

rsmith added inline comments. Comment at: clang/include/clang/Sema/TemplateDeduction.h:338 public: - TemplateSpecCandidateSet(SourceLocation Loc, bool ForTakingAddress = false) : Loc(Loc), ForTakingAddress(ForTakingAddress) {} Do we need to allow the loc

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

rsmith added a comment. Sorry for the late comments. Comment at: clang/lib/Sema/SemaInit.cpp:167-177 } else if (ParenExpr *PE = dyn_cast(E)) { E = PE->getSubExpr(); } else if (UnaryOperator *UO = dyn_cast(E)) { assert(UO->getOpcode() == UO_Extension);

[PATCH] D150875: Make dereferencing a void* a hard-error instead of warn-as-error

rsmith added a comment. In D150875#4353384 , @erichkeane wrote: > We are the only of the major compilers with this "extension" (I hesitate to > call it that, as I'm not sure this FITS in the 'extension's permitted by > standard) I'm not objecting to r

[PATCH] D149713: [Sema] Avoid emitting warnings for constant destruction.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM with the extra test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149713/new/ https://reviews.llvm.org/D149713 ___

[PATCH] D149713: [Sema] Avoid emitting warnings for constant destruction.

rsmith added a comment. I think this would be an interesting test: struct S { /*not constexpr*/ S(); constexpr ~S() {} }; S s; // no warning struct T { /*not constexpr*/ T(); constexpr ~T() { if (b) {} } bool b; }; T t; // expected-warning {{exit-time destructor

[PATCH] D149514: Check if First argument in _builtin_assume_aligned_ is of pointer type

rsmith added a comment. There's some corresponding code in `lib/CodeGen` that can be deleted too: case Builtin::BI__builtin_assume_aligned: { const Expr *Ptr = E->getArg(0); Value *PtrValue = EmitScalarExpr(Ptr); - if (PtrValue->getType() != VoidPtrTy) - PtrValue

[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

rsmith added a comment. This looks like a nice improvement to me. Comment at: clang/lib/Interpreter/IncrementalParser.cpp:162 + if (P->getCurToken().is(tok::annot_repl_input_end)) { +P->ConsumeAnyToken(); // FIXME: Clang does not call ExitScope on finalizing the regul

[PATCH] D149516: [Sema] `setInvalidDecl` for error deduction declaration

rsmith added a comment. I wonder if there are any cases where the "deduction guide" is so broken that we shouldn't be creating a `CXXDeductionGuideDecl` at all. For example, if there's no trailing return type, then the declaration is syntactically invalid (and looks more like a constructor than

[PATCH] D148274: [clang] Fix overly aggressive lifetime checks for parenthesized aggregate initialization

rsmith added a comment. LGTM, I'll leave approval to @shafik :) Comment at: clang/lib/Sema/SemaInit.cpp:5379 +} else { ArrayLength = Args.size(); +} Can we assert that we have an `IncompleteArrayType` here? (That is, that we don't have a `Depend

[PATCH] D149264: GH62362: Handle constraints on "Using" inherited constructors

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:138-144 +bool SkipForSpecialization, +bool ForConstraintInstantiation) { if (!ClassTemplSpec->isClassScopeExplicitSpecialization()) { // We

[PATCH] D149264: GH62362: Handle constraints on "Using" inherited constructors

rsmith added a comment. I think that the bugs that this was aiming to fix were fixed by rG1e43349e321694d7fee3d77cb691887ad67fb5d7 , which means that we should not ask whether the constraints of an inherited constructor are s

[PATCH] D148372: [clang] add diagnose when member function contains invalid default argument

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Are there any calls to `Sema::ActOnParamDefaultArgumentError` left? If not, can you also remove the function? Otherwise this LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D148274: [clang] Fix overly aggressive lifetime checks for parenthesized aggregate initialization

rsmith added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:5364-5368 if (const ConstantArrayType *CAT = S.getASTContext().getAsConstantArrayType(Entity.getType())) ArrayLength = CAT->getSize().getZExtValue(); else ArrayLength = Args

[PATCH] D148372: [clang] add diagnose when member function contains invalid default argument

rsmith added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:7385 if (!ConsumeAndStoreInitializer(*DefArgToks, CIK_DefaultArgument)) { DefArgToks.reset(); +Diag(ArgStartLoc, diag::err_expected) << "initializer"; I th

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

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:746 + diag::note_template_class_instantiation_here) +<< CTD << Active->InstantiationRange; } else { This diagnostic won't include the templ

[PATCH] D147782: [clang] Fix 'operator=' lookup in nested class

rsmith added a comment. Ah, I see. In D147782#4251185 , @J-Camilleri wrote: > 2. Have the `IdResolver` declarations ordered by scope, going from most > nested to least nested. This is the intended behavior. But `Sema::PushOnScopeChains` doesn't correc

[PATCH] D147782: [clang] Fix 'operator=' lookup in nested class

rsmith added a comment. What *should* happen here is that the lookup for `operator=` in `Inner::invokeAssign` should walk up the enclosing `Scope`s and `DeclContext`s, traversing both the results from the `IdResolver` and the results from `DeclContext` lookups. First, we should look in the scop

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. In D147655#4251042 , @aaron.ballman wrote: > In D147655#4250922 , @royjacobson > wrote: > >> In D147655#4250056 , @rsmith wrote: >> >>> There has

[PATCH] D147673: [Clang] Improve designated inits diagnostic location

rsmith added inline comments. Comment at: clang/test/SemaCXX/cxx2b-designated-initializers.cpp:13 + const S result { // expected-error {{field designator (null) does not refer to any field in type 'const S'}} +.a = x + }; Why are we rejecting this? Repos

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. In D147655#4248800 , @royjacobson wrote: > I agree it doesn't affect too much code, but people do instantiate templates > manually sometimes. IIUC, linking against shared libraries would break if > that library does explicit ins

[PATCH] D147744: [clang][Sema] Add MultiLevelTemplateArgumentList::dump()

rsmith added inline comments. Comment at: clang/include/clang/Sema/Template.h:265 + +LLVM_DUMP_METHOD void dump() const { + LangOptions LO; I think it would be useful to also print out the number of retained outer levels. Repository: rG LLVM Github

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

rsmith added a comment. In D147722#4249612 , @erichkeane wrote: > Hmmm... the examples I thought would cause a problem don't seem to when > switching that to Done. So I'm going to run regression tests and give that a > shot. > > I think I now get what

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

rsmith added a comment. In D146897#4249300 , @rjmccall wrote: > The third idea seems like a valuable warning, but it's basically a > *different* warning and shouldn't be lumped into this one. I understand the > instinct to carve it out here so that we

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2854-2858 -template <> -bool DeducedArgsNeedReplacement( -ClassTemplatePartialSpecializationDecl *Spec) { - return !Spec->isClassScopeExplicitSpecialization(); -} It's not obv

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added inline comments. Comment at: clang/include/clang/AST/ExprConcepts.h:502 ArrayRef LocalParameters, + SourceLocation RParenLoc, ArrayRef Requirements, erichkeane wrote: > Is this an unrelated change? Are p

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

rsmith added a comment. I'm happy whenever Aaron is. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8809 + // + // We will support P2448R2 in language modes earlier than C++23 as an extenion + // The concept of constexpr-compatible was removed. =

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rsmith added a comment. Here's the diff to `test_demangle.pass.cpp`: --- a/libcxxabi/test/test_demangle.pass.cpp +++ b/libcxxabi/test/test_demangle.pass.cpp @@ -29995,10 +29995,10 @@ const char* cases[][2] = {"_Z15test_uuidofTypeI10TestStructEvDTu8__uuidofT_EE", "void test_uuidofTyp

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15656 case BO_XorAssign: -DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false); CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S); break; python3kgae wrote: > rsmith wrote: > > Thi

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15656 case BO_XorAssign: -DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false); CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S); break; This is the same thing, but for `this->x +

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

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Does my horrible lambda example work now? If so, that seems like a useful testcase. Comment at: clang/lib/Sema/SemaConcept.cpp:781 + /*ForConstraintInstantiation=*/true

[PATCH] D147068: Fix merging of member-like constrained friends across modules.

rsmith closed this revision. rsmith added a comment. Refactoring and test landed in rGa07abe27b4d1d39ebb940a7f4e6235302444cbf0 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1470

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

rsmith added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:775-776 +static bool IsInsideImplicitFullTemplateSpecialization(const DeclContext *DC) { + auto *CTSDecl = dyn_cast_or_null( + DC->getOuterLexicalRecordContext()); + return CTSDecl && !isa(CTSDecl) &&

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

rsmith added a comment. Can you also change www/cxx_status.html to list P2103R0 as supported in the most recent version of Clang rather than in some previous version? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146178/new/ https://reviews.llvm.o

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

rsmith added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:775-776 +static bool IsInsideImplicitFullTemplateSpecialization(const DeclContext *DC) { + auto *CTSDecl = dyn_cast_or_null( + DC->getOuterLexicalRecordContext()); + return CTSDecl && !isa(CTSDecl) &&

[PATCH] D147281: Stop modifying trailing return types.

This revision was automatically updated to reflect the committed changes. Closed by commit rGaeee4ebd6891: Stop modifying trailing return types. (authored by rsmith). Changed prior to commit: https://reviews.llvm.org/D147281?vs=509840&id=510321#toc Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D147068: Fix merging of member-like constrained friends across modules.

rsmith added a comment. Hm, looks like rGdb1b827ecfed6 landed the actual functioal change here, making this just a no-functional-change refactoring plus an added test, so I'm going to go ahead and land it. Repository: rG

  1   2   3   4   5   6   7   8   9   10   >