[PATCH] D147281: Stop modifying trailing return types.

rsmith created this revision. rsmith added reviewers: erichkeane, ChuanqiXu. Herald added a project: All. rsmith requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This change reverts the functional change from D144626

[PATCH] D144626: [C++20] [Modules] Trying to compare the trailing require clause of the primary template when performing ODR checking

rsmith added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:6704 +}; +const FunctionDecl *PrimaryX = TryToGetPrimaryTemplatedFunction(FuncX); +const FunctionDecl *PrimaryY = TryToGetPrimaryTemplatedFunction(FuncY); If this is necessary

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

rsmith updated this revision to Diff 509818. rsmith added a comment. - Merge branch 'main' into merge-constrained-friends Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147068/new/ https://reviews.llvm.org/D147068 Files:

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbc73ef0031b5: PR60985: Fix merging of lambda closure types across modules. (authored by rsmith). Changed prior to commit:

[PATCH] D146465: [clang] Fix 2 bugs with parenthesized aggregate initialization

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

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

rsmith added a comment. This all looks good to me. Sorry for the delay. Comment at: clang/lib/Sema/SemaConcept.cpp:728 bool SkipForSpecialization = false) { MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(

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

rsmith created this revision. rsmith added a reviewer: aaron.ballman. Herald added a project: All. rsmith requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. When a friend declaration has a requires-clause, and either it's a non-template

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

rsmith added a comment. In D145737#4202945 , @rsmith wrote: > In D145737#4202351 , @aaron.ballman > wrote: > >> Checking: are the libc++ precommit CI failures related to these changes? >> It's a

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

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:3586-3591 + // MSVC treats all predefined expressions as string literals rather than char + // arrays. + if (LangOpts.MicrosoftExt) +return SL; + return PredefinedExpr::Create(Context, Loc, ResTy,

[PATCH] D146465: [clang] Fix 2 bugs with parenthesized aggregate initialization

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:1582-1596 + MultiExprArg ExprsToPass; + if (Exprs.size() == 1 && isa(Exprs[0])) { +// C++20 [expr.static.cast]p4: +// An expression E can be explicitly converted to a type T...if T is an +//

[PATCH] D146465: [clang] Fix 2 bugs with parenthesized aggregate initialization

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:1582-1596 + MultiExprArg ExprsToPass; + if (Exprs.size() == 1 && isa(Exprs[0])) { +// C++20 [expr.static.cast]p4: +// An expression E can be explicitly converted to a type T...if T is an +//

[PATCH] D146041: Fix weirdly apologetic diagnostic messages

rsmith added a comment. I have no strong opinions on the merits of this patch in either direction; I think the "sorry"s in the Sema diagnostics for regrettable non-conformance make Clang marginally friendlier, but they do nothing to actually help people who encounter the diagnostic. FWIW, the

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

rsmith added a comment. In D145737#4202351 , @aaron.ballman wrote: > Checking: are the libc++ precommit CI failures related to these changes? It's > a modules-specific build config, so I figured it's worth double-checking. I took a quick glance when I

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

rsmith added a comment. The approach of attempting to adjust the depth here is not correct, and we should rip this whole mechanism out and reimplement it. Consider this closely related testcase: template concept Concept = true; template struct A { template V> void C(V&&

[PATCH] D146164: Fix nomerge attribute not working with __builtin_trap().

rsmith added a comment. FYI: this attribute appears to not work on (at least) x86 and arm currently, because the backend also does some merging: https://godbolt.org/z/d43M83oax Comment at: clang/lib/CodeGen/CGExpr.cpp:3626-3627 } - + if (InNoMergeAttributedStmt) +

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

rsmith added a comment. > Based on feedback we will provide users to the ability to downgrade this this > diagnostic to a waring to allow for a transition period. We expect to turn > this diagnostic to an error in the next release. Can we revert this now? Repository: rG LLVM Github

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

rsmith added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:7364 ReadingKindTracker ReadingKind(Read_Decl, *this); + Deserializing D(this); shafik wrote: > Curious, why do we need this here and below. I'm actually not sure how we got

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

rsmith updated this revision to Diff 505631. rsmith marked an inline comment as done. rsmith added a comment. - Add `=` to parameter name comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145737/new/ https://reviews.llvm.org/D145737 Files:

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

rsmith added a comment. Please also update the P2448 row in cxx_status.html and add release notes. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9410-9421 +def ext_incorrect_defaulted_comparison_constexpr : Extension<

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

rsmith added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:1778-1780 + void setLambdaNumbering(Decl *ContextDecl, unsigned IndexInContext, + unsigned ManglingNumber, + unsigned DeviceManglingNumber,

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

rsmith updated this revision to Diff 505240. rsmith marked 6 inline comments as done. rsmith added a comment. - Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145737/new/ https://reviews.llvm.org/D145737 Files:

[PATCH] D145859: [Clang][CodeGen] Fix linkage of template parameter objects

rsmith added a comment. Looks good. If you want to look at the visibility side of this separately I think that's fine (I only called it out here because we often consider them at the same time). Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3241-3246 +

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

rsmith added a comment. In D124351#4185007 , @aaron.ballman wrote: > In the reproducer from Sam, there are three lambdas: > `create::Create([] {});` in `main.cpp`, but this does not participate in > AST merging because it's within the main source file

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

rsmith updated this revision to Diff 504296. rsmith added a comment. - Add some missing recursive deserialization guards. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145737/new/ https://reviews.llvm.org/D145737 Files:

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

rsmith created this revision. Herald added a subscriber: martong. Herald added a reviewer: shafik. Herald added a project: All. rsmith requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Previously, distinct lambdas would get merged, and

[PATCH] D145034: [Clang][Sema] Start fixing handling of out-of-line definitions of constrained templates

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good to me. This is fixing an important bug, and while there's more cleanup I'd like for us to do, it seems important to get this fix landed first. My understanding is that the release

[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates

rsmith added inline comments. Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:113-114 + if (TemplateParamLists.size() == 1) { +// FIXME: pick the correct template parameter list based on NNS, SS +// and getCurScope(). +

[PATCH] D145034: [Clang][Sema] Preparations to fix handling of out-of-line definitions of constrained templates

rsmith added a comment. This direction looks promising to me. Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1677-1678 CXXScopeSpec Spec; +if (TemplateInfo.TemplateParams) + Spec.setTemplateParamLists(*TemplateInfo.TemplateParams); + I think we'll

[PATCH] D144889: [C2x] Support in freestanding

rsmith added a comment. Do we know what GCC intends to do about this C change? Per their documentation , they intend for GCC to be / eventually become a complete freestanding implementation without need for a separate libc: > GCC aims towards

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16824-16841 if (InnerCond && isa(InnerCond)) { // Drill down into concept specialization expressions to see why they // weren't satisfied. Diag(StaticAssertLoc,

[PATCH] D143670: Stop claiming we support [[carries_dependency]]

rsmith added a comment. Until or unless a C++ DR permits us to define `__has_cpp_attribute(carries_dependency)` to any value other than `200809L`, we have a conformance requirement to macro-expand this to that value. CWG2695 only adds a note, so it changes nothing in this regard. Similarly, we

[PATCH] D143533: [clang] Allow gnu::aligned attribute to work with templated type alias declarations

rsmith added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:2154 + if (uint64_t Result = SubstTypeVisitor(Ctx, SubType->getReplacementType()) +.TryEval(Attr->getAlignmentExpr())) { +TI.Align = Result; I don't think

[PATCH] D143546: [clang-format] Insert a space between a numeric UDL and a dot

rsmith added a comment. I wonder if this can be fixed more generally by using `TokenConcatenation::AvoidConcat` to determine whether `clang-format` should require a space between two tokens. This is the logic that `clang -E` uses when printing preprocessed tokens to avoid token splices. For

[PATCH] D129951: adds `__disable_adl` attribute

rsmith added inline comments. Comment at: clang/include/clang/Basic/Attr.td:4132 +def DisableADL : InheritableAttr { + let Spellings = [Keyword<"__disable_adl">]; + let Subjects = SubjectList<[Function]>; Has this syntax been discussed already? If not, why did

[PATCH] D140584: [Clang] Refactor "Designators" into a unified implementation [NFC]

rsmith accepted this revision. rsmith added a comment. I think this is fine as a short-term stepping stone to a different representation. Comment at: clang/include/clang/AST/Designator.h:88 + /// An array designator, e.g., "[42] = 0" and "[42 ... 50] = 1". + template

[PATCH] D142384: [C++20] Fix a crash with modules.

rsmith added a comment. In D142384#4102937 , @hans wrote: > I don't know if that's expected or not, but maybe that behavior change could > be used as an inspiration for a test case. At least this suggests a way to test this with a C++ unit test: get a

[PATCH] D140584: [Clang] Refactor "Designators" into a unified implementation [NFC]

rsmith added inline comments. Comment at: clang/include/clang/AST/Designator.h:88 + /// An array designator, e.g., "[42] = 0" and "[42 ... 50] = 1". + template struct ArrayDesignatorInfo { +/// Location of the first and last index expression within the designated

[PATCH] D140584: [Clang] Refactor "Designators" into a unified implementation [NFC]

rsmith added inline comments. Comment at: clang/include/clang/AST/Designator.h:88 + /// An array designator, e.g., "[42] = 0" and "[42 ... 50] = 1". + template struct ArrayDesignatorInfo { +/// Location of the first and last index expression within the designated

[PATCH] D142384: [C++20] Fix a crash with modules.

rsmith added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:557-560 + RecordDecl::field_iterator field_begin() const { +assert(hasDefinition() && "Definition not available to get fields."); +return static_cast(getDefinition())->field_begin(); + }

[PATCH] D142384: [C++20] Fix a crash with modules.

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:6233 + assert(RD->hasDefinition()); + RD->getDefinition(); if (RD->getNumVBases()) { I think it would make sense to use the definition of the class as `RD` here, since we're going to

[PATCH] D142704: [C++20][Modules] Handle template declarations in header units.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:15265 FD->getFormalLinkage() == Linkage::ExternalLinkage && - !FD->isInvalidDecl() && BodyKind != FnBodyKind::Delete && + !FD->isInvalidDecl() && !IsFnTemplate && BodyKind !=

[PATCH] D142733: Add _Optional as fast qualifier

rsmith added a comment. Including a link to the RFC (https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/68004/2) in each of the patches in this series would be helpful. Assuming that we want to go in this direction, it seems quite expensive to model

[PATCH] D140250: Define NULL in its own header

rsmith added a comment. In D140250#4081102 , @rsmith wrote: > Our builtin header `stddef.h` shouldn't be built as a module. It > fundamentally needs to be treated as a textual header, because it consumes > macros defined by the includer. The module map

[PATCH] D140250: Define NULL in its own header

rsmith added a comment. In D140250#4081009 , @iana wrote: > In D140250#4080990 , @rsmith wrote: > >> Wait a second... if an OS wants only `NULL`, we already have a supported way >> of achieving that, which is

[PATCH] D140250: Define NULL in its own header

rsmith added a comment. Wait a second... if an OS wants only `NULL`, we already have a supported way of achieving that, which is compatible with GCC and glibc and other POSIX compilers -- define `__need_NULL` before including the header. We shouldn't be providing internal headers with

[PATCH] D141441: [clang] Add ElaboratedType sugaring for types on implicit special members

This revision was automatically updated to reflect the committed changes. Closed by commit rG028d13b15612: [clang] Add ElaboratedType sugaring for types on implicit special members (authored by brad.king, committed by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D141441: [clang] Add ElaboratedType sugaring for types on implicit special members

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This makes sense to me, and it looks like the memory impact from the extra type nodes should be relatively small. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

rsmith added a comment. In D141625#4052820 , @dblaikie wrote: > Yeah, might be nice to document where the instability comes from - if it > comes from a DenseMap or similar, then a test that fails either in forward or > reverse iteration mode would be

[PATCH] D131858: [clang] Track the templated entity in type substitution.

rsmith added a comment. In D131858#3957630 , @arphaman wrote: > This change has caused a failure in Clang's stage 2 CI on the green dragon > Darwin CI: > https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6390/console. > > Assertion failed:

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

rsmith added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6198-6199 + dyn_cast(DestType->getAs()->getDecl()); + S.getLangOpts().CPlusPlus20 && RD && RD->isAggregate() && Failed() && + getFailureKind() == FK_ConstructorOverloadFailed &&

[PATCH] D140584: [Clang] Refactor "Designators" into a unified implementation [NFC]

rsmith added inline comments. Comment at: clang/include/clang/AST/Designator.h:88 + /// An array designator, e.g., "[42] = 0" and "[42 ... 50] = 1". + template struct ArrayDesignatorInfo { +/// Location of the first and last index expression within the designated

[PATCH] D140584: [Clang] Refactor "Designators" into a unified implementation [NFC]

rsmith added inline comments. Comment at: clang/include/clang/AST/Designator.h:87-92 +/// Refers to the index expression. +Expr *IndexExpr; + +/// Location of the first index expression within the designated +/// initializer expression's list of subexpressions. +

[PATCH] D127284: [clang-repl] Support statements on global scope in incremental mode.

rsmith added inline comments. Comment at: clang/lib/Parse/Parser.cpp:1032-1034 +// FIXME: Remove the incremental processing pre-condition and verify clang +// still can pass its test suite, which will harden +// `isDeclarationStatement`. v.g.vassilev

[PATCH] D127284: [clang-repl] Support statements on global scope in incremental mode.

rsmith added a comment. This is looking good to me. No further non-trivial concerns on my side. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6165 + // Stop squashing the top-level stmts into a single function. + if (CurCGF &&

[PATCH] D137751: Produce a more informative diagnostics when Clang runs out of source locations

rsmith closed this revision. rsmith added a comment. Landed in rG9e52db182794d87bb8527dda313afd1ac491ab11 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137751/new/ https://reviews.llvm.org/D137751

[PATCH] D137751: Produce a more informative diagnostics when Clang runs out of source locations

rsmith added a comment. In D137751#3930663 , @aaron.ballman wrote: > LGTM! Please add a release note when landing so folks know about the > improvements here. Thanks! Thanks, done and landed. CHANGES SINCE LAST ACTION

[PATCH] D127284: [clang-repl] Support statements on global scope in incremental mode.

rsmith added inline comments. Comment at: clang/lib/AST/Decl.cpp:5264 + +FunctionDecl *TopLevelStmtDecl::getOrConvertToFunction() { + if (FD) I would hope that we can remove this. Instead, I think we can teach `CodeGen` to emit a sequence of

[PATCH] D137751: Produce a more informative diagnostics when Clang runs out of source locations

rsmith added a comment. In D137751#3919162 , @alexfh wrote: > I wonder whether we can include SLoc usage information into `-print-stats` > output? Yeah, we could extend the source manager information there with something like this. It's a bit awkward

[PATCH] D137751: Produce a more informative diagnostics when Clang runs out of source locations

rsmith updated this revision to Diff 474385. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137751/new/ https://reviews.llvm.org/D137751 Files: clang/test/Driver/aix-ld.c Index: clang/test/Driver/aix-ld.c === ---

[PATCH] D137751: Produce a more informative diagnostics when Clang runs out of source locations

rsmith created this revision. rsmith added a reviewer: aaron.ballman. Herald added a subscriber: mgrang. Herald added a project: All. rsmith requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. If Clang runs out of source location address

[PATCH] D127284: [clang-repl] Support statements on global scope in incremental mode.

rsmith added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:-5568 + if (SS.isNotEmpty() && SS.getScopeRep()) { +NestedNameSpecifier *NNS = SS.getScopeRep(); +if (NNS->getAsNamespace() || NNS->getAsNamespaceAlias()) { + TPA.Revert(); + return

[PATCH] D137251: [clang][cuda/hip] Allow `__noinline__` lambdas

rsmith added a comment. In D137251#3902835 , @tra wrote: > LGTM in principle. Please wait for @rsmith's OK. I'm happy to defer to @aaron.ballman on this :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D136803: [clang] Include the type of a pointer or reference non-type template parameter in its notion of template argument identity.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp:592 + + // FIXME: We will need to mangle these cases differently too! + int m; Should

[PATCH] D136397: [Clang] Change AnonStructIds in MangleContext to per-function based

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good to me. Comment at: clang/include/clang/AST/Mangle.h:97 + +// If FunctionDecl is passed in, the anonnousstructID will be per-function +// based.

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

rsmith added inline comments. Comment at: clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp:31 IP<> ip7; // expected-error{{non-type template argument of type 'int *' is not a constant expression}} +IP<(int*)1> ip8; // expected-error {{non-type template argument does not

[PATCH] D134772: [Clang] Make constraints of auto part of NTTP instead of AutoType

rsmith added a comment. I'm somewhat skeptical of this approach, because constrained `auto` can appear in places within the type of an NTTP other than the top level -- for example, in `template`. (Clang is non-conforming and doesn't support this yet, but it will need to do so eventually.) In

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652 + Expr *FirstArg = TheCall->getArg(0); + if (auto *CE = dyn_cast(FirstArg)) +FirstArg = CE->getSubExprAsWritten(); rjmccall wrote: > yronglin wrote: > > rjmccall wrote: >

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652 + Expr *FirstArg = TheCall->getArg(0); + if (auto *CE = dyn_cast(FirstArg)) +FirstArg = CE->getSubExprAsWritten(); This looks very suspicious to me: this will remove a

[PATCH] D127284: [clang-repl] Support statements on global scope in incremental mode.

rsmith added a comment. In terms of the high-level direction here, I think it would make sense to approach this by adding a full-fledged language extension to Clang to allow statements at the top level (with a `-f` flag to enable it), and then enable that extension in the interpreter. The

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa002063de37c: Enforce module decl-use restrictions and private header restrictions in textual… (authored by rsmith). Changed prior to commit:

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

rsmith updated this revision to Diff 458312. rsmith edited the summary of this revision. rsmith added a comment. - Help text update from Aaron. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132779/new/ https://reviews.llvm.org/D132779 Files:

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

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:126 + `_ are now + diagnosed even when the includer is a textual header. aaron.ballman wrote: > You should mention

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

rsmith updated this revision to Diff 457742. rsmith added a comment. Document the flag to undo the behavior, and mention that it's going away. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132779/new/ https://reviews.llvm.org/D132779 Files:

[PATCH] D133229: [driver] Prune module-map related flags, if they are not going to be needed

rsmith added a comment. I've landed some tests for the specific functionality that intends to use modules info under `-fno-modules` in b484256f59850e702df4d4532c5f31f478879bb9 . I think the approach you're taking here is

[PATCH] D132801: [driver] Additional ignoring of module-map related flags, if modules are disabled

rsmith added a comment. This doesn't look right to me -- we still use module maps when modules are disabled to enforce layering checking, and when `-fmodules-local-submodule-visibility` is enabled but `-fmodules` is disabled we'll use them to provide modular semantics without pre-building

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

rsmith updated this revision to Diff 456077. rsmith added a comment. Add release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132779/new/ https://reviews.llvm.org/D132779 Files: clang/docs/ReleaseNotes.rst

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

rsmith created this revision. rsmith added reviewers: Bigcheese, aaron.ballman. Herald added a project: All. rsmith requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Per the documentation, these restrictions were intended to apply to textual

[PATCH] D132550: Changes to code ownership in clang and clang-tidy

rsmith accepted this revision. rsmith added a comment. LGTM Thank you! Comment at: clang/CODE_OWNERS.TXT:117 N: Richard Smith E: rich...@metafoo.co.uk +D: Emeritus owner Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rsmith accepted this revision. rsmith added a comment. A handful more comments, but I think we've basically converged here. I'm happy to take another look after you address these if you'd like (or you could ask someone else for a final pass), or for you to land this once you're happy. Before

[PATCH] D128372: [Clang-Tidy] Empty Check

rsmith added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:23 +#include "llvm/ADT/SmallVector.h" +#include + Comment at: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp:149 +

[PATCH] D130308: WIP: [clang] extend getCommonSugaredType to merge sugar nodes

rsmith added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12654 + return QualType(); +// FIXME: It's inneficient to have to unify the original types. +return Ctx.getAdjustedType(Ctx.getCommonSugaredType(OX, OY), Underlying); Typo

[PATCH] D129951: [clang] teaches Clang the special ADL rules for functions in std::ranges

rsmith added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:9438 // FIXME: Pass in the explicit template arguments? ArgumentDependentLookup(Name, Loc, Args, Fns); It would seem preferable to me to do the filtering in

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

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

[PATCH] D111509: [clang] use getCommonSugar in an assortment of places

rsmith accepted this revision. rsmith added a comment. Looks good to me. Comment at: clang/lib/Sema/SemaExprCXX.cpp:6504-6516 // If we have function pointer types, unify them anyway to unify their // exception specifications, if any. if

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

rsmith added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:12177 +return X; + assert(X->getCanonicalDecl() == Y->getCanonicalDecl()); + // FIXME: Could return the earliest declaration between those two. Comment at:

[PATCH] D127284: [WIP] [clang-repl] Support statements on global scope in incremental mode.

rsmith added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:5247-5248 + // ObjC + case tok::at: +return getLangOpts().ObjC; + In Objective-C, both declarations and expressions can start with `@`. In general we'd need to look at the next token

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rsmith added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:10733-10736 // For enums, get the underlying integer type of the enum, and let the general // integer type signchanging code handle it. if (const auto *ETy = T->getAs()) T =

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:8694-8696 + Diag(NewDecl->getLocation(), isa(Old) + ? diag::err_redefinition + : diag::err_redefinition_different_kind)

[PATCH] D126694: [C++20][Modules] Implementation of GMF decl elision.

rsmith added inline comments. Comment at: clang/include/clang/Sema/Sema.h:2275-2276 + llvm::SmallPtrSet SeenDecls; + llvm::SmallPtrSet SeenTypes; + These names seem too general to live directly in `Sema`. Comment at:

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. A few comments, but they're all minor things or FIXMEs. I'm happy for this to land once they're addressed. Comment at: clang/include/clang/AST/DeclBase.h:229 +/// This

[PATCH] D128083: [C++20] Correctly handle constexpr/consteval explicitly defaulted special member instantiation behavior

rsmith added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:1315-1316 !FieldRec->hasConstexprDefaultConstructor() && !isUnion()) // The standard requires any in-class initializer to be a constant // expression. We consider this to be a

[PATCH] D126960: [clang][sema] Unary not boolean to int conversion

rsmith added a comment. Thank you for the patch! This is certainly an improvement but I think there are still some cases where we compute the wrong range for `~` with this patch applied. Comment at: clang/lib/Sema/SemaChecking.cpp:12328 return

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rsmith added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:9265-9266 + ? Context.getBitIntType(!IsMakeSigned, Context.getIntWidth(BaseType)) + : Context.getIntTypeForBitwidth(Context.getIntWidth(BaseType), +

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rsmith added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:5863 // FIXME: derive from "Target" ? - return WCharTy; + return IntTy; } This change seems surprising. Can you explain what's going on here? Comment at:

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This is a bold direction but I like it a lot. Over to @aaron.ballman for final approval. Comment at: clang/lib/Sema/ParsedAttr.cpp:228 + switch (getParsedKind()) { + case

[PATCH] D127351: clang: fix early substitution of alias templates

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1872 QualType Result = getSema().Context.getTemplateTypeParmType( - T->getDepth() - TemplateArgs.getNumSubstitutedLevels(), T->getIndex(), - T->isParameterPack(), NewTTPDecl); +

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

rsmith added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:242-246 +Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs, +GNUAttrs, ); } else { -Decl =

[PATCH] D127075: [clang] P2266: apply move elision rules on throw expr nested in function prototypes

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaExprCXX.cpp:848 (Scope::FnScope | Scope::ClassScope | Scope::BlockScope | - Scope::FunctionPrototypeScope

[PATCH] D126757: Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

rsmith added inline comments. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2040 + /* Traverse base definition for explicit specializations */ \ + TRY_TO(Traverse##DECLKIND##Helper(D)); \ +} else {

[PATCH] D126757: Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

rsmith added inline comments. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2040 + /* Traverse base definition for explicit specializations */ \ + TRY_TO(Traverse##DECLKIND##Helper(D)); \ +} else {

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