[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

rsmith added a comment. In https://reviews.llvm.org/D49511#1206265, @rsmith wrote: > In https://reviews.llvm.org/D49511#1194716, @leonardchan wrote: > > > @rsmith any more feedback on this current version? If it still looks > > incorrect to use the record this way, I don't mind simplifying it to

[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

rsmith added a comment. In https://reviews.llvm.org/D49511#1194716, @leonardchan wrote: > @rsmith any more feedback on this current version? If it still looks > incorrect to use the record this way, I don't mind simplifying it to work on > lvalue to rvalue conversions without checking for a lea

[PATCH] D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr().

rsmith added a comment. I don't think this is NFC. Testcase: long long int a, b, c, d; unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, &(++d)); } Today, Clang increments `a`, `b`, `c`, and `d` twice each in `f()`. https://reviews.llvm.org/D50979

[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

rsmith added inline comments. Comment at: lib/Parse/ParseExpr.cpp:1126 + +Actions.StartCheckingNoDeref(); + This parser-driven start/stop mechanism will not work in C++ templates. Instead, can you remove the "start" part and check the noderef exprs as part o

[PATCH] D41217: [Concepts] Concept Specialization Expressions

rsmith added inline comments. Comment at: include/clang/AST/ExprCXX.h:4420 + /// \brief The concept named. + ConceptDecl *NamedConcept; + saar.raz wrote: > rsmith wrote: > > You should also track the `FoundDecl` and the optional > > `NestedNameSpecifierLoc` (j

[PATCH] D50783: [CodeGen] Merge identical block descriptor global variables

rsmith added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:271-276 llvm::GlobalVariable *global = -elements.finishAndCreateGlobal("__block_descriptor_tmp", - CGM.getPointerAlign(), - /*constant*/ t

[PATCH] D50805: Don't warn on returning the address of a label from a statement expression

rsmith added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6927-6928 } else if (isa(L)) { -Diag(DiagLoc, diag::warn_ret_addr_label) << DiagRange; +if (LK == LK_Return) + Diag(DiagLoc, diag::warn_ret_addr_label) << DiagRange; } els

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

rsmith added a comment. In https://reviews.llvm.org/D50740#1203126, @arphaman wrote: > In https://reviews.llvm.org/D50740#1202248, @jkorous wrote: > > > Hi Alex, nice work! > > > > I am just wondering if it would be beneficial to assert Loc and End are in > > the same file. It might help to catc

[PATCH] D50805: [Sema] Don't warn on returning the address of a label

rsmith added a comment. In https://reviews.llvm.org/D50805#1201910, @rnk wrote: > I think the state machine use case is real, though, something like: > > void *f(void *startlabel) { > common_work(); > goto *startlabel; > state1: > return &&state2; > state2: > return &&state3

[PATCH] D50805: Don't warn on returning the address of a label

rsmith added a comment. There is no guarantee that you can use an address-of-label value from one function invocation in another invocation of the same function. GCC's documentation says it's the user's own problem to prevent inlining and cloning if the program requires an address-of-label exte

[PATCH] D50683: [Android] Set NewAlign for 64-bit Android to 8 bytes

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. This doesn't seem necessary. `NewAlign` specifies the alignment beyond which types acquire "new-extended alignment" per the C++ standard, or equivalently the alignment beyond which w

[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type

rsmith added a comment. (Just writing up my archaeology and research so no-one else needs to do it...) We used to intentionally keep the `Type` bitfields 32 bits long. However, these commits (accidentally, as far as I can tell) took us past 32 bits for the type bitfields: https://reviews.llvm.o

[PATCH] D50666: Fix Stmt::ignoreImplicit

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/AST/Stmt.cpp:121-122 - if (auto *bte = dyn_cast(s)) -s = bte->getSubExpr(); +if (auto *ewc = dyn_cast(s)) + s = ewc->getSubExpr(); -

[PATCH] D50526: Model type attributes as regular Attrs

This revision was automatically updated to reflect the committed changes. rsmith marked an inline comment as done. Closed by commit rC339623: Model type attributes as regular Attrs. (authored by rsmith, committed by ). Changed prior to commit: https://reviews.llvm.org/D50526?vs=160207&id=160463

[PATCH] D50526: Model type attributes as regular Attrs

rsmith added inline comments. Comment at: include/clang/Basic/Attr.td:1510 + let Spellings = [Keyword<"__unsafe_unretained">]; + let Documentation = [Undocumented]; +} aaron.ballman wrote: > I don't suppose you can throw in some quick docs for this keyword? Or

[PATCH] D50526: Model type attributes as regular Attrs

rsmith updated this revision to Diff 160207. rsmith marked 10 inline comments as done. https://reviews.llvm.org/D50526 Files: include/clang/AST/ASTContext.h include/clang/AST/Attr.h include/clang/AST/Type.h include/clang/AST/TypeLoc.h include/clang/Basic/Attr.td include/clang/Sema/Sem

[PATCH] D47757: [Sema] Produce diagnostics when unavailable aligned allocation/deallocation functions are called

rsmith added inline comments. Comment at: test/SemaCUDA/call-host-fn-from-device.cu:88 __host__ __device__ void class_specific_delete(T *t, U *u) { - delete t; // ok, call sized device delete even though host has preferable non-sized version + delete t; // expected-error {{re

[PATCH] D47757: [Sema] Produce diagnostics when unavailable aligned allocation/deallocation functions are called

rsmith added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:3463 + DiagnoseUseOfDecl(OperatorNewOrDelete, TheCall->getExprLoc()); + Are we also missing a `MarkFunctionReferenced` call here? (I don't think it matters much for the predefined new/delete,

[PATCH] D50531: [NFC] Convert ParsedAttr to use llvm::TrailingObjects

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good, thanks. (I'm not sure why `PropertyData` needs special handling rather than being treated as two `IdentifierLoc` arguments, but that's not made any worse here, just perhaps a bit m

[PATCH] D50527: [Parser] Support alternative operator token keyword args in Objective-C++

rsmith added a comment. One comment, but otherwise the code change looks mechanically correct. Not accepting only because I don't know whether this is the intended language rule for Objective-C++ or not (please get someone else to sign off on that). Comment at: clang/lib/Pars

[PATCH] D50418: [Sema] Support for P0961R1: Relaxing the structured bindings customization point finding rules

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM with a small bugfix. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1122 +for (Decl *D : MemberGet) { + if (FunctionTemplateDecl *FTD = dyn_cast(D)) { +Temp

[PATCH] D50526: Model type attributes as regular Attrs

rsmith created this revision. rsmith added a reviewer: aaron.ballman. rsmith added a project: clang. Specifically, `AttributedType` now tracks a regular `attr::Kind` rather than having its own parallel `Kind` enumeration, and `AttributedTypeLoc` now holds an `Attr*` instead of holding an ad-hoc

[PATCH] D50418: [Sema] Support for P0961R1: Relaxing the structured bindings customization point finding rules

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1118-1130 +// ... and if that finds at least one declaration that is a function +// template whose first template parameter is a non-type parameter ... +LookupResult::Filter Filter = MemberGe

[PATCH] D50088: [Sema] Fix an error with C++17 auto non-type template parameters

rsmith added a comment. Can you also add a test where the `auto` is not top-level (eg, `template`) and a test using `decltype(auto)`? https://reviews.llvm.org/D50088 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D43357: [Concepts] Function trailing requires clauses

rsmith added inline comments. Comment at: lib/Sema/SemaDecl.cpp:8377-8381 + } else if (D.hasTrailingRequiresClause()) { +// C++2a [class.virtual]p6 +// A virtual method shall not have a requires-clause. +Diag(NewFD->getTrailingRequiresClause()->getLoc

[PATCH] D50291: [C++] Delay checking of constexpr-ness for special members.

rsmith added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:6045 // Inform the class that we've finished declaring this member. Record->finishedDefaultedOrDeletedMember(M); M->setTrivialForCall( Your new handling should go

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

rsmith added inline comments. Comment at: lib/AST/DeclTemplate.cpp:203 + +Expr *TemplateDecl::getAssociatedConstraints() { + return getOrCollectAssociatedConstraints(getASTContext(), Rather than producing an `Expr*` (which will presumably eventually need to con

[PATCH] D41217: [Concepts] Concept Specialization Expressions

rsmith added inline comments. Comment at: include/clang/AST/ExprCXX.h:4420 + /// \brief The concept named. + ConceptDecl *NamedConcept; + You should also track the `FoundDecl` and the optional `NestedNameSpecifierLoc` (just like a `DeclRefExpr` would). Clang-b

[PATCH] D40381: Parse concept definition

rsmith added inline comments. Comment at: include/clang/AST/DeclTemplate.h:3007 +/// \brief Definition of concept, not just declaration actually. +class ConceptDecl : public TemplateDecl { This comment isn't appropriate. Please just describe what the node is. (

[PATCH] D43357: [Concepts] Function trailing requires clauses

rsmith added a comment. Looking promising, but this patch will need some rework: you need to track the trailing requires clause on the `Declarator` itself, not on the `DeclChunk`, because it's not part of the `DeclChunk` (and may appear in contexts where there is no function chunk).

[PATCH] D50349: Port getStartLoc -> getBeginLoc

rsmith added a comment. +Hans (release manager for LLVM 7) Hans, this patch series will affect the API of common Clang classes, resulting in patches to Clang SVN needing some (mechanical) modifications to be applied to the Clang 7 release branch if we land it now. What do you think about that?

[PATCH] D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls

This revision was automatically updated to reflect the committed changes. rsmith marked an inline comment as done. Closed by commit rC338945: Avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end… (authored by rsmith, committed by ). Repository: rC Clang https://review

[PATCH] D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:521 + CGM.getCodeGenOpts().OptimizationLevel > 0 && + !CGM.getCodeGenOpts().SanitizeAddressUseAfterScope) { +OldConditional = OutermostConditional;

[PATCH] D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls

rsmith updated this revision to Diff 159153. https://reviews.llvm.org/D50286 Files: lib/CodeGen/CGExpr.cpp test/CodeGenCXX/conditional-temporaries.cpp test/CodeGenCXX/lifetime-asan.cpp Index: test/CodeGenCXX/lifetime-asan.cpp

[PATCH] D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls

rsmith updated this revision to Diff 159143. rsmith added a comment. Add forgotten test file. https://reviews.llvm.org/D50286 Files: lib/CodeGen/CGExpr.cpp test/CodeGenCXX/conditional-temporaries.cpp test/CodeGenCXX/lifetime-asan.cpp Index: test/CodeGenCXX/lifetime-asan.cpp =

[PATCH] D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls

rsmith created this revision. rsmith added a reviewer: rjmccall. When a non-extended temporary object is created in a conditional branch, the lifetime of that temporary ends outside the conditional (at the end of the full-expression). If we're inserting lifetime markers, this means we can end up

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

rsmith added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1036 +return; + // That's it. We can't rule out any more cases with the data we have. + rsmith wrote: > lebedev.ri wrote: > > rsmith wrote: > > > I don't like the overlap between the impl

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

rsmith added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1036 +return; + // That's it. We can't rule out any more cases with the data we have. + lebedev.ri wrote: > rsmith wrote: > > I don't like the overlap between the implicit truncation chec

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

rsmith added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1036 +return; + // That's it. We can't rule out any more cases with the data we have. + I don't like the overlap between the implicit truncation check and this check. I think you should

[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

This revision was automatically updated to reflect the committed changes. rsmith marked 2 inline comments as done. Closed by commit rC338464: [P0936R0] add [[clang::lifetimebound]] attribute (authored by rsmith, committed by ). Changed prior to commit: https://reviews.llvm.org/D49922?vs=158149&

[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

rsmith marked 3 inline comments as done. rsmith added inline comments. Comment at: lib/Sema/SemaInit.cpp:6959 + case IndirectLocalPathEntry::LifetimeBoundCall: +// FIXME: Consider adding a note for this. +break; aaron.ballman wrote: > Is this

[PATCH] D38320: [clang] Fix serializers for `TypeTemplateParmDecl` + related types

rsmith added inline comments. Comment at: lib/Serialization/ASTReaderDecl.cpp:3448-3456 if (!inheritDefaultTemplateArgument(Context, FTTP, ToParam)) break; } else if (auto *FNTTP = dyn_cast(FromParam)) { if (!inheritDefaultTemplateArgument(Context, FNT

[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

rsmith added inline comments. Comment at: include/clang/Basic/AttrDocs.td:2371 +is retained by the return value of the annotated function +(or, for a constructor, in the value of the constructed object). +It is only supported in C++. aaron.ballman wrote: > I read

[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

rsmith updated this revision to Diff 158149. rsmith marked 8 inline comments as done. https://reviews.llvm.org/D49922 Files: include/clang/AST/Type.h include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td lib/AST/Type.cpp lib/AST/TypePri

[PATCH] D50002: [coroutines] Fix handling of dependent co_await in StmtProfiler

This revision was automatically updated to reflect the committed changes. Closed by commit rL338343: [coroutines] Fix handling of dependent co_await in StmtProfiler. (authored by rsmith, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

rsmith added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137 + overflow happens (signed or unsigned). + Both of these two issues are handled by ``-fsanitize=implicit-conversion`` + group of checks. - ``-fsanitize=unreachable``: If control

[PATCH] D49865: Inform the AST of #pragma FENV_ACCESS use

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Parse/ParsePragma.cpp:619-623 +#if NOTYET // FIXME: Add this cli option when it makes sense. + case tok::OOS_DEFAULT: +FPC = getLangOpts().getDefault

[PATCH] D50002: [coroutines] Fix handling of dependent co_await in StmtProfiler

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good, thanks. Do you need someone to commit this for you? Repository: rC Clang https://reviews.llvm.org/D50002 ___ cfe-commits mailing l

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

rsmith accepted this revision. rsmith added a comment. Only comments on documentation and assertions. Feel free to commit once these are addressed to your satisfaction. Comment at: docs/ReleaseNotes.rst:310 + + Just like ``-fsanitize=integer``, these issues are **not** undefi

[PATCH] D49848: Parse a possible trailing postfix expression suffix after a fold expression

rsmith added a comment. Thanks, I like this approach a lot more. Comment at: include/clang/Parse/Parser.h:1658 +CastExpr,// Also allow '(' type-name ')' +FoldExpr // Also allow fold-expression }; This should be reordered up nearer t

[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

rsmith added a comment. In https://reviews.llvm.org/D49511#1170693, @leonardchan wrote: > Done. I opted for not using `ExpressionEvaluationContextRecord` because I > don't think I was using it correctly. A few other tests in sema failed when I > tried pushing and popping from the stack holding

[PATCH] D49439: [Sema] Fix a crash while converting constructors to deduction guides

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaTemplate.cpp:1899-1907 // Canonicalize the type. This (for instance) replaces references to // typedef members of the current ins

[PATCH] D49865: Inform the AST of #pragma FENV_ACCESS use

rsmith added a comment. Thanks, this is definitely a step in the right direction. Comment at: include/clang/AST/Expr.h:3257-3261 + // Get the FENV_ACCESS status of this operator. Only meaningful for + // operations on floating point types. + bool isFENVAccessOn() const { +

[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

rsmith added inline comments. Comment at: include/clang/AST/TypeLoc.h:96-97 /// Convert to the specified TypeLoc type, returning a null TypeLoc if - /// this TypeLock is not of the desired type. It will consider type - /// adjustments from a type that wad written as a T to a

[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

rsmith created this revision. rsmith added a reviewer: aaron.ballman. Herald added a reviewer: javed.absar. Herald added a subscriber: kristof.beyls. This patch adds support for a new attribute, [[clang::lifetimebound]], that indicates that the lifetime of a function result is related to one of t

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

rsmith added inline comments. Comment at: docs/ReleaseNotes.rst:49-51 +- A new Implicit Cast Sanitizer (``-fsanitize=implicit-cast``) group was added. + Please refer to the :ref:`release-notes-ubsan` section of the release notes + for the details. Regarding the

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

rsmith added a comment. This looks good, with some minor changes. Please add more test coverage, though, specifically: - test all four forms of lambda that we recognize after `delete` - add a test that the FixItHint is correct (maybe in test/FixIt/fixit-cxx0x.cpp, which checks that applying the

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

rsmith added inline comments. Comment at: include/clang/Sema/Sema.h:4040-4041 // explicit nested-name-specifier). - void MarkAnyDeclReferenced(SourceLocation Loc, Decl *D, bool MightBeOdrUse); + void MarkAnyDeclReferenced(SourceLocation Loc, Decl *D, bool MightBeOdrUse, +

[PATCH] D49844: [AST] Add a isActuallyImplicitCast() helper to the CastExpr class.

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. What I requested was that either we make `CastExpr::isPartOfExplicitCast()` return `true` for `CastExpr`s that are not `ImplicitCastExpr`s, or that we move `isPartOfExplicitCast` do

[PATCH] D49838: [AST] Sink 'part of explicit cast' down into ImplicitCastExpr

rsmith added inline comments. Comment at: include/clang/AST/Expr.h:2928 + bool getIsPartOfExplicitCast() const { +return CastExprBits.PartOfExplicitCast; Please also rename this to`isPartOfExplicitCast` as requested on IRC. Comment at: i

[PATCH] D49848: Parse a possible trailing postfix expression suffix after a fold expression

rsmith added a comment. Shouldn't the caller of ParseParenExpression already be doing this? Repository: rC Clang https://reviews.llvm.org/D49848 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Driver/ToolChains/Darwin.cpp:2027 + isAlignedAllocationUnavailable()) CC1Args.push_back("-faligned-alloc-unavailable"); } Quux

[PATCH] D49729: [AST][1/4] Move the bit-fields from TagDecl, EnumDecl and RecordDecl into DeclContext

rsmith added a comment. I've not done a full review, but the approach here looks good, thanks! Repository: rC Clang https://reviews.llvm.org/D49729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rsmith added a comment. Does this check destructors of nested aggregate initializations in the case where brace elision occurs? I don't think just checking the top level of the SK_ListInitialization is enough. Perhaps it would make more sense to mark the dtors used from InitListChecker (in non-

[PATCH] D49508: [Sema] Mark implicitly-inserted ICE's as being part of explicit cast (PR38166)

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Sema/SemaCast.cpp:94-101 +void updatePartOfExplicitCastFlags(CastExpr *CE) { + // Walk down from the CE to the OrigSrcExpr, and mark all immediat

[PATCH] D49067: Stop wrapping __has_include in another macro

rsmith added inline comments. Comment at: include/__config:153 +#ifndef __has_include +#define __has_include(...) 0 #endif arichardson wrote: > EricWF wrote: > > I do prefer not hijacking this name, but if it's needed to make things > > work, then it's OK with

[PATCH] D49508: [Sema] Mark implicitly-inserted ICE's as being part of explicit cast (PR38166)

rsmith added inline comments. Comment at: include/clang/AST/Stmt.h:206 +bool PartOfExplicitCast : 1; +unsigned BasePathSize : 32 - 6 - 1 - NumExprBits; }; lebedev.ri wrote: > rjmccall wrote: > > lebedev.ri wrote: > > > rjmccall wrote: > > > > This need

[PATCH] D48862: [OpenEmbedded] Fix lib paths for OpenEmbedded targets

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Driver/ToolChains/Gnu.cpp:2205-2209 + + // Deal with OpenEmbedded linux sysroots (like for arm-oe-linux-gnueabi) + // which are of the form /usr

[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

rsmith added a comment. The way in which you're checking for the problematic cases is unnecessarily expensive. Instead of performing a separate AST traversal, please detect whether you should be producing the warning directly when forming the problematic expressions. (For example, you could sto

[PATCH] D49457: DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array

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 rC337422: DR330: when determining whether a cast casts away constness, consider (authored by rsmith, committed by ). Reposi

[PATCH] D49457: DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array

rsmith added inline comments. Comment at: lib/Sema/SemaCast.cpp:535 +T1 = Unwrap(T1); +T2 = Unwrap(T2).withCVRQualifiers(T2.getCVRQualifiers()); + } rjmccall wrote: > Hmm. Just CVR? I understand that we can have problems here with the > enumerated qua

[PATCH] D49439: [Sema] Fix a crash while converting constructors to deduction guides

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:1899-1907 // Canonicalize the type. This (for instance) replaces references to // typedef members of the current instantiations with the definitions of // those typedefs, avoiding triggering

[PATCH] D49457: DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array

rsmith created this revision. rsmith added a reviewer: rjmccall. DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array. For example, this allows casting from pointer to array ofarray of const volat

[PATCH] D48266: [Driver] Add -fno-digraphs

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good with one cleanup. Comment at: lib/Frontend/CompilerInvocation.cpp:2177-2178 + if (const Arg *A = Args.getLastArg(OPT_fdigraphs, OPT_fno_digraphs)) +Opts.Dig

[PATCH] D49227: Override a bit fields layout from an external source

This revision was automatically updated to reflect the committed changes. Closed by commit rC337047: Use external layout information to layout bit-fields for MS ABI. (authored by rsmith, committed by ). Repository: rL LLVM https://reviews.llvm.org/D49227 Files: lib/AST/RecordLayoutBuilder.c

[PATCH] D49227: Override a bit fields layout from an external source

This revision was automatically updated to reflect the committed changes. Closed by commit rL337047: Use external layout information to layout bit-fields for MS ABI. (authored by rsmith, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D

[PATCH] D49227: Override a bit fields layout from an external source

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: test/CodeGenCXX/override-bit-field-layout.cpp:5-12 +struct S { + short a : 3; + short b : 5; +}; + +void use_structs() { + S ss[sizeof(S)]; ---

[PATCH] D37442: [C++17] Disallow lambdas in template parameters (PR33696).

rsmith accepted this revision. rsmith added inline comments. Comment at: include/clang/Sema/Sema.h:4000-4004 + ExpressionEvaluationContextRecord::ExpressionKind Type = ExpressionEvaluationContextRecord::EK_Other); enum ReuseLambdaContextDecl_t { ReuseLambdaContextDecl };

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

rsmith added a comment. Thanks! Comment at: lib/Parse/ParseExprCXX.cpp:2956 +const Token Next = GetLookAheadToken(2); +const auto GetAfter = [this] { return GetLookAheadToken(3); }; + I don't think this lambda is useful: since you're not caching the res

[PATCH] D48266: [Driver] Add -fno-digraphs

rsmith added inline comments. Comment at: include/clang/Driver/Options.td:1337-1340 +def fdigraphs : Flag<["-"], "fdigraphs">, Group, Flags<[CC1Option]>, + HelpText<"Enable alternative token representations '<:', ':>', '<%', '%>', '%:' (default)">; +def fno_digraphs : Flag<["-"

[PATCH] D45712: Diagnose invalid cv-qualifiers for friend decls.

rsmith added a comment. Can we avoid the duplication by putting this check in `Sema::ParsedFreeStandingDeclSpec` instead? Repository: rC Clang https://reviews.llvm.org/D45712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

[PATCH] D48894: [AST] Rename some Redeclarable functions to reduce confusion

rsmith added inline comments. Comment at: include/clang/AST/Redeclarable.h:106 -mutable llvm::PointerUnion Next; +mutable llvm::PointerUnion Prev; I think this is still a confusing name, because it points either to the previous declaration or to the

[PATCH] D48863: [Sema] Explain coroutine_traits template in diag

rsmith added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9051 def err_implied_coroutine_type_not_found : Error< "%0 type was not found; include before defining " "a coroutine">; Maybe we should also remove the "%0 type was not

[PATCH] D48863: [Sema] Explain coroutine_traits template in diag

rsmith added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9053 "a coroutine">; +def note_coroutine_types_for_traits_here : Note< + "the coroutine traits class template is being instantiated using the return " modocache wrote: > GorN

[PATCH] D48322: [Sema] Discarded statment should be an evaluatable context

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D48322#1150316, @erik.pilkington wrote: > Is this what you were concerned about? Yes, exactly. Thank you for checking. Repository: rC Clang https://reviews.ll

[PATCH] D48880: [Sema] Fix crash in getConstructorName.

rsmith added a comment. Looks fine, but please put the test case somewhere more appropriate (under SemaCXX, for instance). Comment at: lib/Sema/SemaExprCXX.cpp:117 } assert(InjectedClassName && "couldn't find injected class name"); We would be able to

[PATCH] D48036: [CUDA] Make min/max shims host+device.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks right to me (other than the missing `constexpr` in C++14 onwards). Though this is subtle enough that I suspect the only way to know for sure is to try it. https://reviews.llvm.org/D4803

[PATCH] D48322: [Sema] Discarded statment should be an evaluatable context

rsmith added a comment. Hmm, so this will mean that we can have internal linkage declarations marked `Used` for which there is no definition, and we need to not warn on that. I think it might be better to avoid marking things used in this case (even though they're still technically odr-used).

[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works

rsmith added a comment. This is not specific to the `ASTImporter`; any change to the AST after a call to `getParents` would have similar problems. Generally, responsibility for dealing with this must lie with the consumer of the parent map, not with the `ASTContext`, since the `ASTContext` gene

[PATCH] D47103: Implement strip.invariant.group

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: llvm/docs/LangRef.rst:12928 +established by ``invariant.group`` metadata no longer holds, to obtain a new pointer +value that does carries fresh invariant gr

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. > I'm not 100% thrilled that we're emitting two warnings about the same thing > for slightly different reasons; alternatives welcome. :) Me either, but given our policy that warning flags just

[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647 + } +} + Prazek wrote: > rjmccall wrote: > > Prazek wrote: > > > rjmccall wrote: > > > > Incidentally, how do you protect against code like this? > > > > > > > > A *ptr;

[PATCH] D48571: improve diagnostics for missing 'template' keyword

This revision was automatically updated to reflect the committed changes. Closed by commit rC335687: Diagnose missing 'template' keywords in more cases. (authored by rsmith, committed by ). Repository: rC Clang https://reviews.llvm.org/D48571 Files: include/clang/Basic/BitmaskEnum.h inclu

[PATCH] D48571: improve diagnostics for missing 'template' keyword

rsmith created this revision. rsmith added a reviewer: dblaikie. Herald added a subscriber: eraman. We track when we see a name-shaped expression followed by a `<` token that we parse as a comparison. Then: - if we see a token sequence that cannot possibly be an expression but can be a template

[PATCH] D48100: Append new attributes to the end of an AttributeList.

rsmith added a comment. This patch appears to have at least caused us to process attributes too many times in some cases. For example: __attribute__((noreturn,noinline,unused)) void f(); before your patch gives this with `clang -Xclang -ast-dump`: `-FunctionDecl 0xccef1e0 <:1:1, col:50> co

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

rsmith added inline comments. Comment at: lib/Parse/ParseExprCXX.cpp:2907-2909 +const Token Next = GetLookAheadToken(2); +const Token After = GetLookAheadToken(3); +const Token Last = GetLookAheadToken(4); Please don't request the additional lookahead

[PATCH] D37442: [C++17] Disallow lambdas in template parameters (PR33696).

rsmith accepted this revision. rsmith added a comment. Please also change the `EnterExpressionEvaluationContext` in `TreeTransform::TransformTemplateArgument` to specify `EK_TemplateArgument` (though that doesn't actually matter for the lambda diagnostics because we'll find all the problems in

[PATCH] D39679: [C++11] Fix warning when dropping cv-qualifiers when assigning to a reference with a braced initializer list

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In any case, this looks good to me. It'll look even better with the `FK_AddressOfUnaddressableFunction` case fixed :) https://reviews.llvm.org/D39679 __

[PATCH] D39679: [C++11] Fix warning when dropping cv-qualifiers when assigning to a reference with a braced initializer list

rsmith added a comment. In https://reviews.llvm.org/D39679#1037591, @Rakete wrote: > Note: I didn't change `Args[0]` to `OnlyArg` in > `FK_AddressOfUnaddressableFunction`, because I'm pretty sure that C++ doesn't > have unaddressable functions and thus there is no need to decompose an > in

[PATCH] D47956: [MS] Consder constexpr globals to be inline, as in C++17

rsmith added a comment. Can we now remove the corresponding MSVC-specific hacks elsewhere (eg, `ASTContext::isMSStaticDataMemberInlineDefinition`), or do we still need those for `const`-but-not-`constexpr` static data members? https://reviews.llvm.org/D47956

<    13   14   15   16   17   18   19   20   21   22   >