[PATCH] D79617: Add cet.h for writing CET-enabled assembly code

rsmith added a comment. I would like a specification for this header to be added somewhere. We shouldn't be implementing random things with no specification. (Suppose someone claims that our `` is wrong in some way. How would we know whether they're right?) Ideally, I'd also like this header

[PATCH] D79895: Add a new warning to warn when passing uninitialized variables as const reference parameters to a function

rsmith added inline comments. Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1518 UsesMap uses; + UsesMap constRefUses; zequanwu wrote: > rnk wrote: > > If possible, it would be nice to avoid having a second map. > I use second map to let the new

[PATCH] D80055: Diagnose union tail padding

rsmith added a comment. I'm not convinced that this is an especially useful diagnostic (much like with `-Wpadded`), but I'm also not opposed if you are aware of cases where it would be used. It seems worth pointing out that, at least in C, merely assigning to a union member results in all

[PATCH] D68049: Propeller: Clang options for basic block sections

rsmith added a comment. Clang side LG with some minor changes. Comment at: clang/docs/ClangCommandLineReference.rst:1336 + +Generate labels for each basic block or place each basic block or a subset of basic blocks in its own section + This file is

[PATCH] D79237: [CUDA][HIP] Fix constexpr variables for C++17

rsmith added a comment. In D79237#2039559 , @tra wrote: > In D79237#2039417 , @tra wrote: > > > > > > Bad news -- it breaks the standard C++ library. [...] >

[PATCH] D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms.

rsmith added inline comments. Comment at: clang/lib/AST/FormatString.cpp:407 +if ((isSizeT() || isPtrdiffT()) && +C.getTargetInfo().getTriple().isOSMSVCRT() && +C.getTargetInfo().getTriple().isArch32Bit()) amccarth

[PATCH] D79830: Add support of __builtin_expect_with_probability

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2195 +Value *ExpectedValue = EmitScalarExpr(E->getArg(1)); +Value *Confidence = EmitScalarExpr(E->getArg(2)); +// Don't generate llvm.expect.with.probability on -O0 as the backend

[PATCH] D76420: Prevent IR-gen from emitting consteval declarations

rsmith added inline comments. Comment at: clang/include/clang/AST/Expr.h:1062 +return ConstantExprBits.APValueKind != APValue::None && + ConstantExprBits.APValueKind != APValue::Indeterminate; } Why do we need to treat indeterminate values

[PATCH] D79830: Add support of __builtin_expect_with_probability

rsmith added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:567 BUILTIN(__builtin_expect, "LiLiLi" , "nc") +BUILTIN(__builtin_expect_with_probability, "LiLiLid" , "nc") BUILTIN(__builtin_prefetch, "vvC*.", "nc") I assume we don't have

[PATCH] D68049: Propeller: Clang options for basic block sections

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:114-127 + // -fbasic-block-sections=. The allowed values with this option are: + // {"labels", "all", "", "none"}. + // + // "labels": Only

[PATCH] D78134: [Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaExpr.cpp:601-603 // C++ [conv.lval]p1: // A glvalue of a non-function, non-array type T can be // converted to a prvalue.

[PATCH] D79378: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.

rsmith marked an inline comment as done. rsmith added a comment. In D79378#2019336 , @rjmccall wrote: > Is it reasonable to figure out ahead of time that we can skip the null check > completely? It'd be kindof nice to also take advantage of this at -O0

[PATCH] D79121: Add nomerge function attribute to clang

rsmith added inline comments. Comment at: clang/lib/Sema/SemaStmtAttr.cpp:178 +return true; + return llvm::any_of(S->children(), hasCallExpr); +} zequanwu wrote: > rsmith wrote: > > This will recurse into too much (eg, the bodies of lambdas and blocks, and

[PATCH] D79548: Fix false positive with warning -Wnon-c-typedef-for-linkage

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

[PATCH] D79504: [Clang] Wrong return type of atomic_is_lock_free

rsmith added a comment. Clang's `` uses the builtin as follows: #define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj))) ... which, combined with the builtin returning `int`, results in a call having the wrong type. So there's definitely a bug *somewhere*. I think what

[PATCH] D78607: [local-bounds] Ignore volatile operations

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:175-177 - ``-fsanitize=undefined``: All of the checks listed above other than ``float-divide-by-zero``,

[PATCH] D78655: [HIP] Add -fhip-lambda-host-device

rsmith added a comment. There are two behaviors that seem to make sense: - Treat lambdas as implicitly HD (like constexpr functions) in all CUDA / HIP language modes. I don't think it makes sense for lambdas to become implicitly HD in C++17 simply because they become implicitly `constexpr`,

[PATCH] D79121: Add nomerge function attribute to clang

rsmith added inline comments. Comment at: clang/lib/Sema/SemaStmtAttr.cpp:178 +return true; + return llvm::any_of(S->children(), hasCallExpr); +} This will recurse into too much (eg, the bodies of lambdas and blocks, and unevaluated operands). It would be

[PATCH] D79236: [docs] Regenerate DiagnosticsReference.rst

rsmith added a comment. In D79236#2019005 , @rjmccall wrote: > Okay. So it doesn't just run `ninja docs-clang-html`? That's unfortunate. It didn't last time I looked into this. And http://clang.llvm.org/docs/DiagnosticsReference.html shows the

[PATCH] D79378: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.

rsmith created this revision. rsmith added reviewers: davide, dnsampaio, rjmccall. Herald added a subscriber: hiraditya. Herald added projects: clang, LLVM. This transformation is correct for a builtin call to 'free(p)', but not for 'operator delete(p)'. There is no guarantee that a user

[PATCH] D79236: [docs] Regenerate DiagnosticsReference.rst

rsmith added a comment. In D79236#2018812 , @rjmccall wrote: > In D79236#2018661 , @rsmith wrote: > > > We do generate this document with tblgen already. The problem is that it's > > not automatically regenerated

[PATCH] D79236: [docs] Regenerate DiagnosticsReference.rst

rsmith added a comment. In D79236#2015982 , @rjmccall wrote: > There's no good reason for this file to exist; documenting the warning > options is useful, but the file should be generated as an intermediate output > of the documentation build and then

[PATCH] D79249: [NOT FOR REVIEW] Experimental support for zero-or-trap behavior foruninitialized variables.

rsmith updated this revision to Diff 261511. rsmith added a comment. - The second operand of 'store' is the pointer, not the first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79249/new/ https://reviews.llvm.org/D79249 Files:

[PATCH] D79249: [NOT FOR REVIEW] Experimental support for zero-or-trap behavior foruninitialized variables.

rsmith created this revision. Herald added subscribers: llvm-commits, cfe-commits, jdoerfert, hiraditya. Herald added projects: clang, LLVM. rsmith updated this revision to Diff 261511. rsmith added a comment. - The second operand of 'store' is the pointer, not the first. Add @llvm.trapping.()

[PATCH] D78350: [AST] Build recovery expressions by default for C++.

rsmith added inline comments. Comment at: clang/include/clang/Basic/LangOptions.def:151 -COMPATIBLE_LANGOPT(RecoveryAST, 1, 0, "Preserve expressions in AST when encountering errors") +COMPATIBLE_LANGOPT(RecoveryAST, 1, CPlusPlus, "Preserve expressions in AST when

[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.

rsmith added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:176 CharUnits Alignment; - if (T->isIncompleteType()) { + if (T->getBaseElementTypeUnsafe()->isIncompleteType()) { Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is

[PATCH] D78760: Check a class has a definition before iterating over its base classes

rsmith added a comment. In D78760#2005874 , @ahatanak wrote: > `TagDecl::isDependentType()` is returning true. Oh, sorry, we do already do the thing I was suggesting. But in your testcase the issue is that the base class `B0` is dependent but is also

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

rsmith added inline comments. Comment at: clang/lib/Parse/ParseOpenMP.cpp:64-67 +return Value == unsigned(V); + } + bool operator!=(OpenMPDirectiveKindExWrapper V) const { +return Value != unsigned(V); Comparing against `V.Value` here would seem more

[PATCH] D78760: Check a class has a definition before iterating over its base classes

rsmith added a comment. We should reject a definition of a class that has a non-dependent base class that's an incomplete type. We need non-dependent bases to be complete in order to be able to do name lookup into them when processing the template definition. (The C++ standard permits us to

[PATCH] D77836: [Attribute] Fix noderef attribute false-negatives

rsmith added inline comments. Comment at: clang/lib/Sema/SemaCast.cpp:734 void CastOperation::CheckDynamicCast() { + CheckNoDerefRAII noderef_check(*this); + Please use UpperCamelCase for local variables. Comment at:

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:882-887 + while (const BinaryOperator *BO = dyn_cast(E)) { +if (!BO->isCommaOp()) + break; +E = BO->getRHS(); +E = E->IgnoreParens(); + } vitalybuka wrote: > rsmith wrote:

[PATCH] D76038: PR45000: Let Sema::SubstParmVarDecl handle default args of lambdas in initializers

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. The responses to the various testcases you posted look OK to me. The language rules aren't clear, and we can revisit this if they get clarified in a different direction, but treating lambdas

[PATCH] D68049: Propeller: Clang options for basic block sections

rsmith added a comment. Please add documentation for the new flags. Comment at: clang/include/clang/Driver/Options.td:1974 def fno_function_sections : Flag<["-"], "fno-function-sections">, Group; +def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, Group,

[PATCH] D73846: [PCH] make sure to not warn about unused macros from -D

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks OK as a workaround. Do you know why we consider these to be in the main file? If we could fix that in the source manager, that'd seem preferable. CHANGES SINCE LAST ACTION

[PATCH] D78404: [clang] Implement P0692R1 from C++20 (access checking on specializations)

rsmith added a comment. In D78404#1990461 , @broadwaylamb wrote: > In D78404#1990192 , @rsmith wrote: > > > ... please also test ... > > > > template class TemplateClass3 > > varTemplate3{}; > > > > > > ...

[PATCH] D77753: Change deprecated -fsanitize-recover flag to apply to all sanitizers, not just UBSan.

This revision was automatically updated to reflect the committed changes. Closed by commit rGc8248dc3bb36: Change deprecated -fsanitize-recover flag to apply to all sanitizers, not just… (authored by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78404: [clang] Implement P0692R1 from C++20 (access checking on specializations)

rsmith added a comment. Thanks! Is this really the only case we were getting wrong? If so, great! We should make sure we have test coverage for all the cases affected by P0692R1. Please add some complementary tests for the cases where diagnostics should still be produced. For example, in

[PATCH] D77598: Integral template argument suffix printing

rsmith added a comment. Is is feasible to check whether the corresponding template parameter either has a deduced type or is a parameter of a function template? If not, we don't need to clarify the type of the template argument and could leave off the suffix to shorten the printed argument.

[PATCH] D76038: PR45000: Use Sema::SetParamDefaultArgument in TransformLambdaExpr

rsmith added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:12162 for (unsigned I = 0, NumParams = NewCallOperator->getNumParams(); I != NumParams; ++I) { rsmith wrote: > This logic shouldn't even be here in the first place.. this

[PATCH] D76038: PR45000: Use Sema::SetParamDefaultArgument in TransformLambdaExpr

rsmith added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:12162 for (unsigned I = 0, NumParams = NewCallOperator->getNumParams(); I != NumParams; ++I) { This logic shouldn't even be here in the first place.. this should all be

[PATCH] D78134: [Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

rsmith added a comment. In D78134#1985467 , @ahatanak wrote: > In D78134#1982291 , @rsmith wrote: > > > So far, the direction the wind is blowing is that attempting to perform an > > lvalue-to-rvalue conversion on

[PATCH] D78100: [AST] Suppress the spammy "attempt to use a deleted fucntion" diagnostic.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:12268 /// of sanity. void Sema::ActOnInitializerError(Decl *D) { // Our main concern here is re-establishing invariants like "a hokein wrote: > sammccall wrote: > > rsmith wrote: > > >

[PATCH] D77962: PR38490: Support reading values out of __uuidof(X) expressions during constant evaluation.

rsmith abandoned this revision. rsmith added a comment. Superseded by https://reviews.llvm.org/D78171. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77962/new/ https://reviews.llvm.org/D77962 ___

[PATCH] D78171: Rework how UuidAttr, CXXUuidofExpr, and GUID template arguments and constants are represented.

This revision was automatically updated to reflect the committed changes. Closed by commit rGbab6df86aefc: Rework how UuidAttr, CXXUuidofExpr, and GUID template arguments and constants… (authored by rsmith). Changed prior to commit: https://reviews.llvm.org/D78171?vs=257565=257808#toc

[PATCH] D78171: Rework how UuidAttr, CXXUuidofExpr, and GUID template arguments and constants are represented.

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: clang/lib/AST/Decl.cpp:897-898 +// Fall through. +// FIXME: Should GUIDs receive hidden visibility? We give them DSO-local +// linkage in CodeGen. + rnk wrote: > I

[PATCH] D78172: [www] Update make_cxx_dr_status for v10; regenerate cxx_dr_status.html

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang/www/cxx_dr_status.html:11246 https://wg21.link/cwg1905;>1905 -MAD Dependent types and injected-class-names Haha :)

[PATCH] D75726: [ConstExprPreter] Updated constant interpreter documentation

rsmith accepted this revision. rsmith marked an inline comment as done. rsmith added a comment. This revision is now accepted and ready to land. Thanks! I'm happy for you to go ahead landing patches that implement the direction documented here. (Let me know if you want me to review them anyway,

[PATCH] D78171: Rework how UuidAttr, CXXUuidofExpr, and GUID template arguments and constants are represented.

rsmith created this revision. rsmith added a reviewer: rnk. rsmith added a project: clang. Herald added a subscriber: arphaman. Herald added a reviewer: jdoerfert. rsmith added a comment. There are a few cleanups I made along the way here that I'll be factoring out and committing separately.

[PATCH] D78171: Rework how UuidAttr, CXXUuidofExpr, and GUID template arguments and constants are represented.

rsmith added a comment. There are a few cleanups I made along the way here that I'll be factoring out and committing separately. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78171/new/ https://reviews.llvm.org/D78171

[PATCH] D78068: [www] Turn 'Clang 10' boxes green in C++ status pages to reflect release

rsmith added inline comments. Comment at: clang/www/cxx_dr_status.html:3 "http://www.w3.org/TR/html4/strict.dtd;> Please heed this comment, and check in a matching change to `make_cxx_dr_status` =) Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D78134: [Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

rsmith added a comment. In D78134#1981866 , @rsmith wrote: > I'm going to take this to CWG. So far, the direction the wind is blowing is that attempting to perform an lvalue-to-rvalue conversion on an array should be a no-op. (That's weirdly different

[PATCH] D75951: Keep a list of already-included pragma-once files for mods.

rsmith added inline comments. Comment at: clang/include/clang/Lex/Preprocessor.h:749-750 +/// The set of the included headers' UID for the submodule. +std::set IncludedFiles; + A `std::set` is a very heavy object to be copying each time a module is

[PATCH] D78134: [Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

rsmith added a comment. It seems to me that there's as bug in the C++ specification here. The rules for discarded-value expressions say the lvalue-to-rvalue conversion is applied, whereas the rules for lvalue-to-rvalue conversions say one cannot be applied in this case. It's hard to guess what

[PATCH] D78100: [AST] Suppress the spammy "attempt to use a deleted fucntion" diagnostic.

rsmith added a comment. In D78100#1981620 , @sammccall wrote: > Sorry to go back and forth on this, but I'm not sure whether these > diagnostics are actually spam to be suppressed. I think @adamcz mentioned > these today as reasonable diagnostics we're

[PATCH] D77962: PR38490: Support reading values out of __uuidof(X) expressions during constant evaluation.

rsmith marked 2 inline comments as done. rsmith added a comment. In D77962#1978668 , @rnk wrote: > This code is very string-y. Should clang be parsing uuids into > u32-u16-u16-u8[8] earlier, so that we can get the semantic form directly from > the

[PATCH] D77962: PR38490: Support reading values out of __uuidof(X) expressions during constant evaluation.

rsmith created this revision. rsmith added reviewers: aeubanks, rnk. Herald added a project: clang. Herald added a subscriber: cfe-commits. We try to avoid materializing a full _GUID APValue wherever possible; instead, when accessing an individual field, we only extract the corresponding portion

[PATCH] D77836: [Attribute] Fix noderef attribute false-negatives

rsmith added inline comments. Comment at: clang/lib/Sema/SemaCast.cpp:264 + CheckNoderef(*this, E->getType(), TInfo->getType(), OpLoc); + Warning on this seems reasonable for `static_cast` and `dynamic_cast`, but should `reinterpret_cast` (or the

[PATCH] D76831: [AST] Preserve the DeclRefExpr when it refers to an invalid decl.

rsmith added a comment. Generally I think we should be moving towards finer-grained "invalid" / "contains errors" bits, so that we can preserve as much of the AST as possible and produce accurate diagnostics for independent errors wherever possible. To that end, I think generally the "invalid"

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:882-887 + while (const BinaryOperator *BO = dyn_cast(E)) { +if (!BO->isCommaOp()) + break; +E = BO->getRHS(); +E = E->IgnoreParens(); + } If we're going to further extend

[PATCH] D77697: libc++: adjust modulemap for non-modular C

rsmith added a comment. In D77697#1968998 , @joerg wrote: > This fixes the module build of clang for me. Are you building with or without `-fmodules-local-submodule-visibility` (`cmake -DLLVM_ENABLE_LOCAL_SUBMODULE_VISIBILITY=ON`)? If you don't use

[PATCH] D77753: Change deprecated -fsanitize-recover flag to apply to all sanitizers, not just UBSan.

rsmith created this revision. rsmith added a reviewer: kcc. Herald added a project: clang. Herald added a subscriber: cfe-commits. This flag has been deprecated, with an on-by-default warning encouraging users to explicitly specify whether they mean "all" or ubsan for 5 years (released in Clang

[PATCH] D77502: [clang][CodeGen] Handle throw expression in conditional operator constant folding

This revision was automatically updated to reflect the committed changes. Closed by commit rG878d96011acc: [clang][CodeGen] Handle throw expression in conditional operator constant… (authored by tambre, committed by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D75726: [ConstExprPreter] Updated constant interpreter documentation

rsmith added inline comments. Comment at: clang/docs/ConstantInterpreter.rst:118 Memory management in the interpreter relies on 3 data structures: ``Block`` -object which store the data and associated inline metadata, ``Pointer`` objects -which refer to or into blocks, and

[PATCH] D77586: Allow parameter names to be elided in a function definition in C

rsmith accepted this revision. rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:278 "parameter name cannot have template arguments">; -def err_parameter_name_omitted : Error<"parameter name omitted">; +def

[PATCH] D77502: [clang][CodeGen] Handle throw expression in conditional operator constant folding

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good (with the comment suitably tweaked). Thanks! Comment at: clang/lib/CodeGen/CGExpr.cpp:4334 incrementProfileCounter(expr); + // If a throw expression

[PATCH] D77502: [clang][CodeGen] Handle throw expression in conditional operator constant folding

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:4337 +EmitCXXThrowExpr(ThrowExpr); +return EmitLValue(dead); + } The IR we emit for the dead operand is unreachable; it's a bit wasteful to generate it here and

[PATCH] D68115: Zero initialize padding in unions

rsmith added a comment. In D68115#1962863 , @jfb wrote: > In D68115#1962833 , @rsmith wrote: > > > I think the majority opinion expressed on this review at this point favors > > not guaranteeing

[PATCH] D68115: Zero initialize padding in unions

rsmith added a comment. In D68115#1946990 , @jfb wrote: > In D68115#1946757 , @rsmith wrote: > > > In D68115#1946668 , > > @hubert.reinterpretcast wrote: > > > > > It

[PATCH] D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang

rsmith added a comment. In D77184#1961495 , @thakis wrote: > In D77184#1961220 , @rsmith wrote: > > > In D77184#1961214 , @rsmith wrote: > > > > > In D77184#1961208

[PATCH] D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang

rsmith added a comment. In D77184#1961214 , @rsmith wrote: > In D77184#1961208 , @rsmith wrote: > > > This has broken my ability to run the `check-clang` target on Linux. There > > are symlinks in the path from

[PATCH] D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang

rsmith added a comment. In D77184#1961208 , @rsmith wrote: > This has broken my ability to run the `check-clang` target on Linux. There > are symlinks in the path from which I run my builds, and this patch is > computing incorrect relative paths in that

[PATCH] D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang

rsmith added a comment. This has broken my ability to run the `check-clang` target on Linux. There are symlinks in the path from which I run my builds, and this patch is computing incorrect relative paths in that situation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77342: Sema: check for null TInfo in ActOnBaseSpecifier

rsmith added a comment. I think the problem this aims to address was fixed by rG330873230071ffc2aebc0fe74db55e7a530c2f1b . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77246: Instead of scream, why not roll?

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. I'm sorry, but I can't accept this patch as-is because it removes the `u` suffix from the literal. We're never gonna give `u` up. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5005 +if (Member->getInit() && Member->getInit()->containsErrors()) + Constructor->setInvalidDecl(); if (Member->isBaseInitializer()) rsmith wrote: > This is inappropriate.

[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.

rsmith added inline comments. Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3459-3461 + if (MemInit.get()->getInit() && + MemInit.get()->getInit()->containsErrors()) +AnyErrors = true; The parser should not be inspecting properties of

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:16008-16013 +// In the MS ABI, the complete destructor is implicitly defined, +// even if the base destructor is user defined. +

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rsmith added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:959-963 + /// Indicates if the complete destructor has been implicitly declared + /// yet. Only relevant in the Microsoft C++. + bool definedImplicitCompleteDestructor() const { +return

[PATCH] D76943: [clang analysis] Make mutex guard detection more reliable.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Analysis/ThreadSafety.cpp:2144-2145 + if (auto *CE = dyn_cast(E)) +if (CE->getCastKind() == CK_NoOp || +CE->getCastKind()

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

rsmith added a comment. In D69585#1943222 , @llunak wrote: > In D69585#1942431 , @rsmith wrote: > > > This needs to be done behind a flag. It's an explicit design goal that > > compilation behavior using a PCH or

[PATCH] D68115: Zero initialize padding in unions

rsmith added a comment. In D68115#1946668 , @hubert.reinterpretcast wrote: > It sounds like we are looking for `-fzero-union-padding`. That's been where > the discussion has left off twice for months. I believe the state of Clang prior to this patch

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

rsmith added a comment. This needs to be done behind a flag. It's an explicit design goal that compilation behavior using a PCH or precompiled preamble behaves identically to compilation not using a PCH / precompiled preamble. The behavior of this patch is also non-conforming: we're only

[PATCH] D76323: [AST] Fix handling of long double and bool in __builtin_bit_cast

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:6364-6371 +const llvm::fltSemantics = Info.Ctx.getFloatTypeSemantics(Ty); +unsigned NumBits = APFloat::semanticsSizeInBits(Semantics); +assert(NumBits % 8 == 0); +CharUnits Width =

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

rsmith added a comment. In D76572#1937641 , @Quuxplusone wrote: > [...] take my word for it, you **don't** want to start with the code I wrote > the other day. :) Understood, thanks :) (FWIW, I had a stab at implementing essentially this last year and

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Changes LGTM on the Clang side. In D76572#1935791 , @lebedev.ri wrote: > Passing-by remark: > > > I wrote a Clang warning [not pictured] to diagnose

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

This revision was automatically updated to reflect the committed changes. Closed by commit rGbe10b7e43a3a: Use values cached in ConstantExprs for expression evaluation where present. (authored by wchilders, committed by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/ExprConstant.cpp:7325-7329 + // Override to perform additional checks to ensure the cached APValue + // is actually an LValue. + bool

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:7329 + if (Result.isLValue()) +return Success(Result, E); +} wchilders wrote: > rsmith wrote: > > wchilders wrote: > > > This doesn't seem to be the right answer, and

[PATCH] D76447: Apply ConstantEvaluated evaluation contexts to more manifestly constant evaluated scopes

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16793-16797 + if (isManifestlyEvaluatedVar(*this, D)) { +using ExpressionKind = ExpressionEvaluationContextRecord::ExpressionKind; + +PushExpressionEvaluationContext( +

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

rsmith added inline comments. Comment at: clang/lib/AST/Expr.cpp:2875 - else if (auto *CE = dyn_cast(E)) -return CE->getSubExpr(); - return E; } wchilders wrote: > `IgnoreParensSingleStep` for some reason has been unwrapping `ConstantExpr`s. > This

[PATCH] D76396: Allow immediate invocation of constructors

rsmith added a comment. In D76396#1930773 , @Tyker wrote: > I have already a patch aiming to do the same thing. D74007 > Thanks. Sorry I dropped the ball on that one. =/ Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D76396: Allow immediate invocation of constructors

rsmith added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6466 if (Entity.allowsNRVO()) CurInit = S.BuildCXXConstructExpr(Loc, Step.Type, Step.Function.FoundDecl, It looks like the other callers to

[PATCH] D76096: [clang] allow const structs to be constant expressions in initializer lists

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1013 + if (V->hasInit()) +return Visit(V->getInit(), V->getType()); +return nullptr; efriedma wrote: > rsmith wrote: > > nickdesaulniers wrote: > > > efriedma wrote:

[PATCH] D74238: [clang] Improve diagnostic note for implicit conversion sequences that would work if more than one implicit user-defined conversion were allowed.

rsmith added a comment. How do you defend against attempting an infinite sequence of user-defined conversion? For example: template struct X { operator X(); }; X<0> x0 = X<1>(); Comment at: clang/lib/Sema/SemaOverload.cpp:7333 +// Save the bad conversion in

[PATCH] D76096: [clang] allow const structs to be constant expressions in initializer lists

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1013 + if (V->hasInit()) +return Visit(V->getInit(), V->getType()); +return nullptr; nickdesaulniers wrote: > efriedma wrote: > > You need to be more careful here; we

[PATCH] D76096: [clang] allow const structs to be constant expressions in initializer lists

rsmith added a comment. In D76096#1921842 , @nickdesaulniers wrote: > > The performance implications of deleting those lines is the complicated > > part. > > Where does compile time performance suffer from this? I guess if we have > massive array

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

rsmith added inline comments. Comment at: clang/lib/Serialization/ModuleManager.cpp:183 // Get a buffer of the file and close the file descriptor when done. - Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false); + Buf =

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

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. I'm surprised so few setters needed to be made public for this to work. If this required making lots more setters public, I'd be concerned, because we generally don't want large chunks of the

[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

rsmith added a comment. > There are also no argument expressions, even though the inherited constructor > for `A` takes one argument `x`. The current patch takes argument expressions > from the topmost `CXXConstructExpr`, which also requires a different location > context to access. As far as

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

rsmith added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:629-630 +public: + void setFromASTFile() { FromASTFile = true; } + Setting this after creating a `Decl` is not safe in general; declarations with this flag set have 8 bytes of

<    5   6   7   8   9   10   11   12   13   14   >