[PATCH] D70923: Fix comment to more accurately describe C++ language requirements around tail padding.

rsmith created this revision. rsmith added a reviewer: rjmccall. Herald added a project: clang. As of C++ core issue 43 (http://wg21.link/cwg43), which was voted into the C++ working draft in 1999, it is not permissible to memcpy a base class subobject, even if it's of POD type, so there is no

[PATCH] D46234: Mark if a null statement is the result of constexpr folding

rsmith added a comment. In D46234#1762104 , @xazax.hun wrote: > Just a note for anyone willing to pick this up, PR32203 is also related. Further note: the right approach here would likely be to add a `DiscardedStmt` class deriving from `Stmt` that

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

rsmith added a comment. In D65591#1625744 , @aaron.ballman wrote: > In D65591#1625228 , @ilya-biryukov > wrote: > > > We can also assume they're cheap, use the visitor-based implementation and > > later switch

[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

rsmith added inline comments. Comment at: clang/include/clang/AST/Expr.h:1033-1035 + bool IsCausedByImmediateInvocation() const { +return ConstantExprBits.IsCausedByImmediateInvocation; + } I'd remove the "CausedBy" here -- the `ConstantExpr` is our

[PATCH] D69330: [AST] Add RecoveryExpr to retain expressions on semantic errors

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In D69330#1752089 , @ilya-biryukov wrote: > In D69330#1746137 , @rsmith wrote: > > > I would prefer that we

[PATCH] D70342: [Diagnostics] Put "deprecated copy" warnings into -Wdeprecated-copy

rsmith added a comment. Generally OK, but I'm concerned about including this in `-Wextra`. In particular, the deprecation of copy/move operations when a destructor is explicitly declared has experimentally been found to have a high false-positive rate. Does GCC include this in `-Wextra`? (Its

[PATCH] D69330: [AST] Add RecoveryExpr to retain expressions on semantic errors

rsmith added a comment. I would prefer that we have dedicated tests for them rather than scattering the tests throughout the existing test suite (for example, consider adding `-Wno-unused` to the tests producing "result unused" warnings and adding a dedicated test). Comment

[PATCH] D68407: [WIP][RISCV] Use compiler-rt if no GCC installation detected

rsmith added a comment. In D68407#1744486 , @leonardchan wrote: > Hi. I think this patch is causing some test failures for us: This is breaking us too, in the same way. The "problem" is that this makes the riscv64 target use the compiler-rt crtbegin /

[PATCH] D69360: [NFC] Refactor representation of materialized temporaries

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, looks good subject to some comment fixes + a tiny change to `RecursiveASTVisitor`. Comment at: clang/include/clang/AST/DeclCXX.h:3055 +/// Implicit declartion of

[PATCH] D70190: [clang][modules] Add support for merging lifetime-extended temporaries

rsmith added a comment. Functionally, this looks good, thanks. Comment at: clang/include/clang/Serialization/ASTReader.h:555 + /// Key used to identify LifetimeExtendedTemporaryDecl for merging. + /// containg the lifetime-extending declaration and the mangling number. +

[PATCH] D69935: [DeclCXX] Remove unknown external linkage specifications

rsmith added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:2762 /// ensure a stable ABI for this, we choose the DW_LANG_ encodings /// from the dwarf standard. enum LanguageIDs { Using DWARF encodings here does nothing for AST format

[PATCH] D69792: [NFC] Supress GCC "Bitfield too small to hold all values of enum" warning.

This revision was automatically updated to reflect the committed changes. Closed by commit rG56b5eab12970: [NFC] Supress GCC Bitfield too small to hold all values of enum warning. (authored by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69792: [NFC] Supress GCC "Bitfield too small to hold all values of enum" warning.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, this seems fine to me. (It's unfortunate that the combination of MSVC's behavior and GCC's non-disableable warning forces this upon us, but so be it.) Repository: rG LLVM Github

[PATCH] D41910: [Concepts] Constrained partial specializations and function overloads.

rsmith added a comment. (Didn't finish the review, but I have to run now.) Comment at: include/clang/AST/DeclTemplate.h:742 findSpecializationImpl(llvm::FoldingSetVector , - ArrayRef Args, void *); + void *,

[PATCH] D69518: [Diagnostics] Warn for std::is_constant_evaluated in constexpr mode

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:333 +def warn_is_constant_evaluated_always_true_constexpr : Warning< + "'%0' will always evaluate to 'true'

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: docs/ReleaseNotes.rst:84 + In a future release of Clang, we intend to change the default to + ``-fno-lax-vector-conversions``. + efriedma wrote: > kristof.beyls wrote: > >

[PATCH] D69518: [Diagnostics] Warn for std::is_constant_evaluated in constexpr mode

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8410 + "'std::is_constant_evaluated' will always evaluate to " + "'true' in constexpr mode">, InGroup; def warn_comparison_bitwise_always : Warning< xbolva00 wrote: >

[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

rsmith added a comment. In D63960#1712398 , @Tyker wrote: > The now that constexpr destructors are legal. The code in this patch need to > be adapted, I have question about the following code. > > struct A { > constexpr ~A() {} > }; > >

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: docs/ReleaseNotes.rst:84 + In a future release of Clang, we intend to change the default to + ``-fno-lax-vector-conversions``. + efriedma wrote: > And if you want to allow your

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith added a comment. Ping: is fixed, this is now good to go. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67678/new/ https://reviews.llvm.org/D67678 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D69518: [Diagnostics] Warn for std::is_constant_evaluated in constexpr mode

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8410 + "'std::is_constant_evaluated' will always evaluate to " + "'true' in constexpr mode">, InGroup; def warn_comparison_bitwise_always : Warning< "constexpr mode"

[PATCH] D69360: [NFC] Refactor representation of materialized temporaries

rsmith added a comment. Thanks, I like this a lot. There are a few more changes we'll need in addition to this before we can properly serialize `APValue`s referring to such temporaries: 1. Merging during deserialization. For example, if we have inline int & = 123; in two different modules,

[PATCH] D69479: [Sema] Warn about possible stack exhaution

rsmith added a comment. We should resist using `runWithSufficientStackSpace` where possible. Have you tried making this process data-recursive instead? It looks fairly straightforward to form a worklist of expressions to which we want to apply `AnalyseImplicitConversions`, and run through that

[PATCH] D68818: [hip][cuda] Fix the extended lambda name mangling issue.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/AST/DeclCXX.h:1713 + /// mangling number. + bool hasKnownInternalLinkage() const { +assert(isLambda() && "Not a lambda closure

[PATCH] D41569: [Concepts] Constraint enforcement and diagnostics

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaOverload.cpp:594 }; + struct CNSInfo { +TemplateArgumentList *TemplateArgs; Please add a documentation comment.

[PATCH] D68028: [clang] Add no_builtin attribute

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510 + // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin + // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which + // always returns false before

[PATCH] D68028: [clang] Add no_builtin attribute

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:9508-9510 + // FIXME: We should really be doing this in SemaDeclAttr.cpp::handleNoBuiltin + // but there is a bug with FunctionDecl::isThisDeclarationADefinition() which + // always returns false before

[PATCH] D68818: [hip][cuda] Fix the extended lambda name mangling issue.

rsmith added a comment. Broadly, I think it's reasonable to number additional lambda expressions in CUDA compilations. However: - This is (in theory) an ABI break on the host side, as it changes the lambda numbering in inline functions and function templates and the like. That could be

[PATCH] D41569: [Concepts] Constraint enforcement and diagnostics

rsmith added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:4127 + + case Expr::ConceptSpecializationExprClass: { +// ::= L E # external name These mangling changes look like they could be separated out from the rest of the patch. These plus the

[PATCH] D41284: [Concepts] Associated constraints infrastructure.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Sema/Sema.h:5950 + /// associated constraints of an older declaration of which it is a + /// redeclaration + bool

[PATCH] D41217: [Concepts] Concept Specialization Expressions

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Only minor comments, let me know if you'd like me to take another look after addressing these, or just go ahead and commit. Thanks! Comment at:

[PATCH] D68896: PR43080: Do not build context-sensitive expressions during name classification.

rsmith updated this revision to Diff 224765. rsmith added a comment. Add comments for various NC_ classifications. Remove the unused and broken NC_NestedNameSpecifier classification and the code in ClassifyName that tries to classify names as nested name specifiers. Repository: rC Clang

[PATCH] D68896: PR43080: Do not build context-sensitive expressions during name classification.

rsmith marked an inline comment as done. rsmith added a comment. In D68896#1706870 , @efriedma wrote: > Would it make sense to always use ClassifyName from the parser, instead of > using ActOnIdExpression? I like that idea, at least in principle. We'd

[PATCH] D68849: [Parse] Don't speculatively parse an identifier in the wrong context.

rsmith added a comment. https://reviews.llvm.org/D68896 for the patch to defer forming the `Expr` until it's consumed. This is still not right for implicit member access, but I think that's feasible to fix too (if a little awkward). Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D68896: PR43080: Do not build context-sensitive expressions during name classification.

rsmith created this revision. rsmith added a reviewer: efriedma. Herald added a project: clang. We don't know what context to use until the classification result is consumed by the parser, which could happen in a different semantic context. This covers everything except C++ implicit class member

[PATCH] D68849: [Parse] Don't speculatively parse an identifier in the wrong context.

rsmith added a comment. It looks like it's not actually all that complex to defer building the expression until we're ready to consume the annotation. I've got a mostly-complete patch for that, and I'd like to try finishing that before we add workarounds. Repository: rC Clang CHANGES

[PATCH] D68838: [ARM] Fix arm_neon.h with -flax-vector-conversions=none, part 3

rsmith added a comment. Thanks again! In my testing, this is enough to get the clang testsuite to pass with the default changed to `-flax-vector-conversions=integer`. Comment at: test/CodeGen/aarch64-v8.2a-neon-intrinsics.c:149-170 +uint16x4_t test_vcvt_u16_f16 (float16x4_t

[PATCH] D68824: Fix __builtin_assume_aligned with too large values.

rsmith added a comment. This seems to be missing a CodeGen test for what IR we generate on an overly-large alignment. (The warning says the alignment is ignored, but I don't see where you're actually doing anything different in that case when generating IR.) Comment at:

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith updated this revision to Diff 224473. rsmith added a comment. Test fixup. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67678/new/ https://reviews.llvm.org/D67678 Files: docs/CommandGuide/clang.rst docs/ReleaseNotes.rst

[PATCH] D67160: [clang, ARM] Default to -fno-lax-vector-conversions in ARM v8.1-M.

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. It is not reasonable to make this change on a per-target basis. We should work towards turning lax vector conversions off globally instead. If overload resolution can't distinguish

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/docs/ReleaseNotes.rst:66 + defined if it adds a non-zero offset (or in C, any offset) to a null pointer, + or that forms a null pointer by

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith updated this revision to Diff 224138. rsmith added a comment. Update documentation and release notes. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67678/new/ https://reviews.llvm.org/D67678 Files: docs/CommandGuide/clang.rst docs/ReleaseNotes.rst

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith updated this revision to Diff 224130. rsmith added a comment. Remove special-casing of ARM NEON; let's wait for to get fixed before landing this. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67678/new/ https://reviews.llvm.org/D67678 Files:

[PATCH] D68683: ARM] Fix arm_neon.h with -flax-vector-conversions=none

rsmith added a comment. In D68683#1701769 , @rsmith wrote: > We also have a hack in the ARM and AArch64 target info to set the default for > lax vector conversions back to "all" when NEON is enabled that can now be > removed. Never mind, looks like I

[PATCH] D68581: Include leading attributes in DeclStmt's SourceRange

rsmith added a comment. In D68581#1701787 , @aaron.ballman wrote: > I think this seems like reasonable behavior (for instance, we include the > location of a storage class specifier already), but I am curious if @rsmith > agrees. Yes, I do agree.

[PATCH] D68683: ARM] Fix arm_neon.h with -flax-vector-conversions=none

rsmith accepted this revision. rsmith added a comment. Thank you! We also have a hack in the ARM and AArch64 target info to set the default for lax vector conversions back to "all" when NEON is enabled that can now be removed. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rsmith added a comment. Looks fine to me with some doc improvements. Comment at: clang/docs/ReleaseNotes.rst:64-66 + non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``, + by subtracting pointer's integral value from the pointer itself; in C, also, +

[PATCH] D68055: Add -fgnuc-version= to control __GNUC__ and other GCC macros

rsmith added a comment. From a big-picture perspective, I would like us to move to treating MS and GNU extensions in the same way (at the cc1 level) to the extent that we can. I think this moves us in that direction. One comment, though: I'm not convinced that the `__EXCEPTIONS` macro should

[PATCH] D65695: Implements CWG 1601 in [over.ics.rank/4.2]

This revision was automatically updated to reflect the committed changes. Closed by commit rL373866: Implements CWG 1601 in [over.ics.rank/4.2] (authored by rsmith, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D64820: [Sema] Avoids an assertion failure when an invalid conversion declaration is used

This revision was automatically updated to reflect the committed changes. Closed by commit rL373862: [Sema] Avoids an assertion failure when an invalid conversion declaration is… (authored by rsmith, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed

[PATCH] D64820: [Sema] Avoids an assertion failure when an invalid conversion declaration is used

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64820/new/ https://reviews.llvm.org/D64820 ___ cfe-commits mailing list

[PATCH] D64874: [Sema] Improve handling of function pointer conversions

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, though I'd like to see this generalized to handle noreturn too. Comment at: clang/lib/Sema/SemaTemplate.cpp:6997 + QualType ResultTy; + if

[PATCH] D65695: Implements CWG 1601 in [over.ics.rank/4.2]

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65695/new/ https://reviews.llvm.org/D65695 ___ cfe-commits mailing list

[PATCH] D68364: Prototype implementation of P0784R7: mark all members of std::allocator and std::allocator_traits as constexpr.

rsmith updated this revision to Diff 222944. rsmith added a comment. Undo adding too many files to this change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68364/new/ https://reviews.llvm.org/D68364 Files: libcxx/include/__config

[PATCH] D68364: Prototype implementation of P0784R7: mark all members of std::allocator and std::allocator_traits as constexpr.

rsmith updated this revision to Diff 222943. rsmith added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Moved some changes from the vector patch to here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68364/new/

[PATCH] D67010: [Modules] Move search paths from control block to unhashed control block

rsmith added a comment. This seems reasonable to me, but a test case would be nice. A mode to validate that header path changes don't change anything would be nice (essentially: redo all the header searches with the new set of paths and new filesystem and make sure they find the same thing),

[PATCH] D67249: [Modules][PCH] Hash input files content

rsmith accepted this revision. rsmith 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/D67249/new/ https://reviews.llvm.org/D67249

[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

rsmith added a comment. Very cool, this is an elegant approach. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:57 +def note_consteval_address_accessible : Note< + "%select{pointer|reference}0 on a consteval declaration " + "is not a constant expression">;

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

rsmith added inline comments. Comment at: clang/lib/AST/Expr.cpp:319 case RSK_None: return; case RSK_Int64: Can you use `llvm_unreachable` here? (Are there cases where we use `RSK_None` and then later find we actually have a value to store into the

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good to me, thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67414/new/ https://reviews.llvm.org/D67414 ___ cfe-commits

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:63 +* As per C++ and C Standards (C++: ``[expr.add]``; C17: 6.5.6p8), applying + non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``, aaron.ballman wrote: > rsmith

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:63 +* As per C++ and C Standards (C++: ``[expr.add]``; C17: 6.5.6p8), applying + non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``, In C, even adding 0 to a null

[PATCH] D59754: [Sema] Add c++2a designated initializer warnings

rsmith added a comment. In D59754#1665601 , @thakis wrote: > Another question about this, sorry. Do you know _why_ C++20 is more > restrictive than C99 wrt "mixture of designated and non-designated > initializers in the same initializer list is a C99

[PATCH] D67897: Fix __is_signed builtin

rsmith added a comment. Thanks! Comment at: clang/docs/LanguageExtensions.rst:1165 Note that this currently returns true for enumeration types if the underlying - type is signed, and returns false for floating-point types, in violation of - the requirements for

[PATCH] D67897: Fix __is_signed builtin

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In D67897#1678420 , @zoecarver wrote: > > (Can I interest you in fixing the misbehaviour for enumeration types too?) > > Certainly. You mean that

[PATCH] D67897: Fix __is_signed builtin

rsmith added a comment. In D67897#1678388 , @Quuxplusone wrote: > But `std::is_signed_v` needs to yield `false`. It should yield `true`; the spec says "If is_­arithmetic_­v is true, the same result as T(-1) < T(0); otherwise, false". Repository: rG

[PATCH] D67897: Fix __is_signed builtin

rsmith added a comment. Looks good, but please also update http://clang.llvm.org/docs/LanguageExtensions.html#type-trait-primitives (Can I interest you in fixing the misbehaviour for enumeration types too?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D67399: [ARM] Follow AACPS standard for volatile bitfields

rsmith added a comment. In D67399#1669920 , @rjmccall wrote: > In D67399#1669568 , @jfb wrote: > > > In D67399#1669038 , @dnsampaio > > wrote: > > > > > Indeed our main

[PATCH] D67638: Improve __builtin_constant_p lowering

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. OK, this is fine by me if you're confident this doesn't degrade the generated code at -O0. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67638/new/

[PATCH] D49091: Warn about usage of __has_include/__has_include_next in macro expansions

rsmith added a comment. Under http://eel.is/c++draft/cpp#cond-7.sentence-2, the identifier `__has_include` can't appear anywhere other than in the context of an `#if`, `#elif`, `#iifdef`, or `#ifndef`. That's what we should be checking for and diagnosing here (and we should produce an

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith created this revision. rsmith added a reviewer: SjoerdMeijer. Herald added a subscriber: kristof.beyls. Herald added a project: clang. For ARM compilations with NEON enabled, for now we retain the old default of -flax-vector-conversions=all, because our intrinsic header relies on it, and

[PATCH] D61717: Fix arm_neon.h to be clean under -fno-lax-vector-conversions.

rsmith added a comment. In D61717#1668980 , @SjoerdMeijer wrote: > Do you perhaps have a test case or error that I can look at? Perhaps I or > someone else here can help out a bit here. You can reproduce the problem with, for example: echo

[PATCH] D41217: [Concepts] Concept Specialization Expressions

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: include/clang/Sema/Sema.h:5904 + Expr *ConstraintExpr, + bool ); + (Nit: please align the second

[PATCH] D67515: [Sema][Typo Correction] Fix potential infite loop on ambiguity checks

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: test/Sema/typo-correction-ambiguity.cpp:7 + +namespace AmibguousCorrection +{ Is the typo in "Ambiguous" here intentional? :) Repository:

[PATCH] D67399: [ARM] Follow AACPS standard for volatile bitfields

rsmith added a comment. In D67399#1668759 , @rjmccall wrote: > The exact sequence of volatile accesses is observable behavior, and it's the > ABI's right to define correct behavior for compliant implementations, so we > do need to do this. I'm not

[PATCH] D61717: Fix arm_neon.h to be clean under -fno-lax-vector-conversions.

rsmith abandoned this revision. rsmith added a comment. This patch doesn't work, and the ARM NEON intrinsic header emitter is too opaque and mysterious for me to be able to fix this, and it's not even clear that the generator knows what the actual parameter types of the compiler builtins are.

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

rsmith added a comment. In D67414#1668475 , @mstorsjo wrote: > In D67414#1668443 , @rsmith wrote: > > > Seems very surprising to me that the `gnu_inline` attribute has different > > behavior in their C and C++

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

rsmith added a comment. Seems very surprising to me that the `gnu_inline` attribute has different behavior in their C and C++ frontends. That said, it looks like it's a behavior that they document; their documentation says "In C++, this attribute does not depend on extern in any way, but it

[PATCH] D67429: Improve code generation for thread_local variables:

This revision was automatically updated to reflect the committed changes. Closed by commit rL371767: Improve code generation for thread_local variables: (authored by rsmith, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D67429: Improve code generation for thread_local variables:

rsmith created this revision. rsmith added a reviewer: rjmccall. Herald added a reviewer: jdoerfert. Herald added a project: clang. - Don't bother using a thread wrapper when the variable is known to have constant initialization. - Emit the thread wrapper as discardable-if-unused in TUs that

[PATCH] D59754: [Sema] Add c++2a designated initializer warnings

rsmith added a comment. In D59754#1656217 , @leonardchan wrote: > Hi! We've noticed that for our arm bots, we're getting some flaky builds that > sometimes fail with `error: array designators are a C99 extension > [-Werror,-Wc99-designator]` and

[PATCH] D66361: Improve behavior in the case of stack exhaustion.

rsmith added a comment. In D66361#1655903 , @krytarowski wrote: > This change broke on NetBSD. > > http://lab.llvm.org:8011/builders/netbsd-amd64/builds/22003/steps/run%20unit%20tests/logs/FAIL%3A%20Clang%3A%3Astack-exhaustion.cpp Test disabled for

[PATCH] D59754: [Sema] Add c++2a designated initializer warnings

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL370544: [c++20] Implement semantic restrictions for C++20 designated (authored by rsmith, committed by ). Herald added a

[PATCH] D67023: Diagnose use of ATOMIC_VAR_INIT

rsmith added inline comments. Comment at: lib/Lex/PPMacroExpansion.cpp:523 + llvm::sys::path::filename(getSourceManager().getFilename( + M.getLocalDirective()->getLocation())) == "stdatomic.h") { +Diag(Identifier, diag::warn_pp_macro_deprecated)

[PATCH] D59754: [Sema] Add c++2a designated initializer warnings

rsmith commandeered this revision. rsmith edited reviewers, added: hintonda; removed: rsmith. rsmith added a comment. I'm taking this over to finish it off and submit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59754/new/

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM: while this isn't a solution to the root cause of the issues here, it puts us in a better situation than the status quo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

rsmith added a comment. In D64799#1608085 , @ilya-biryukov wrote: > @rsmith two options for this patch seem to be: > > - silently ignore the errors (behavior before this patch), > - show them to the user at the end of TU (what happens in the current

[PATCH] D66862: Make lround builtin constexpr (and others)

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9617 +APFloat FPVal(0.0); +APSInt IVal(Info.Ctx.getIntWidth(E->getType()), 0); +bool isExact = true; Please use `/*isUnsigned=*/false` for the second argument rather than `0`.

[PATCH] D64820: [Sema] Avoids an assertion failure when an invalid conversion declaration is used

rsmith added inline comments. Comment at: clang/test/Sema/conversion_function_to_function.cpp:1 +// RUN: not %clang_cc1 -fsyntax-only -std=c++14 %s 2>&1 | FileCheck %s + Use `%clang_cc1 -verify` instead of `not %clang_cc1 | FileCheck` Comment

[PATCH] D64820: [Sema] Avoids an assertion failure when an invalid conversion declaration is used

rsmith added inline comments. Comment at: clang/test/Sema/conversion_function_to_function.cpp:1 +// RUN: not %clang_cc1 -fsyntax-only -std=c++14 %s 2>&1 | FileCheck %s + rsmith wrote: > Use `%clang_cc1 -verify` instead of `not %clang_cc1 | FileCheck` This test

[PATCH] D64874: [Sema] Improve handling of function pointer conversions

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:6996 + // a noexcept function can be converted to a noexcept(false) function. + QualType resultTy; + if (getLangOpts().CPlusPlus17 && Please capitalize local variable names

[PATCH] D65695: Implements CWG 1601 in [over.ics.rank/4.2]

rsmith added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:3776 + QualType UnderlyingType = Enum->getIntegerType(); + if (SCS.getToType(1) == UnderlyingType) +return FixedEnumPromotion::ToUnderlyingType; `==` on `QualType` compares the

[PATCH] D66862: Make lround builtin constexpr (and others)

rsmith requested changes to this revision. rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9612 + case Builtin::BIlround: + case Builtin::BI__builtin_lround: { It's non-conforming to accept calls to these non-`__builtin` functions in

[PATCH] D63960: [C++20] Add consteval-specifique semantic

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2317 +def err_invalid_consteval_take_address : Error< + "cannot take address of consteval function %0 in non-constexpr context">; +def err_consteval_address_accessible : Error<

[PATCH] D58497: Clear the KnownModules cache if the preprocessor is going away

rsmith added a comment. Do we need `KnownModules` at all? It seems to serve a very similar purpose to the `Modules` string map on `ModuleMap`. (The main difference seems to be that `KnownModules` can cache module load failures.) If we can keep only a single data structure tracking this, owned

[PATCH] D65694: Properly instantiate a decltype in argument's default initializer

rsmith added a comment. This isn't the right approach: instead of re-instantiating the parameter when we encounter a use of it, we should inject the parameters into the local instantiation scope before trying to instantiate a default argument (`Sema::CheckCXXDefaultArgExpr` should call

[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter

rsmith added a comment. I think we're past the point of large-scale structural comments that are better addressed before the first commit, and it'd be much better to make incremental improvements from here. Please go ahead and commit this, and we can iterate in-tree from now on. Thanks!

[PATCH] D63889: Check possible warnings on global initializers for reachability

rsmith added inline comments. Comment at: clang/include/clang/Parse/Parser.h:1873 + ExprResult ParseInitializer(Decl *DeclForInitializer = nullptr) { +Actions.ExprEvalContexts.back().DeclForInitializer = DeclForInitializer; +ExprResult init; This should

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In D66364#1638026 , @aaron.ballman wrote: > @rsmith are you fine with implementing the diagnostic for these keywords > piecemeal based on the pattern

[PATCH] D66621: [clang] Devirtualization for classes with destructors marked as 'final'

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This seems subtle, but I believe it is correct. I wonder whether we should provide a warning for a non-final class has a final destructor, since moving the `final` from the destructor to the

<    7   8   9   10   11   12   13   14   15   16   >