[PATCH] D33625: [coroutines] Diagnose invalid result types for `await_resume` and `await_suspend` and add missing conversions.

rsmith added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8977-8980 + "the return type of 'await_suspend' is required to be 'void' or 'bool' (have %0)" +>; +def note_await_ready_no_bool_conversion : Note< + "the return type of 'await_ready' is

[PATCH] D29877: Warn about unused static file scope function template declarations.

rsmith added a comment. In https://reviews.llvm.org/D29877#766196, @EricWF wrote: > No. But I can point you to `range-v3` which uses this pattern and I think the > idiom is somewhat appealing, but that's orthogonal to Clang diagnosing it. I found this:

[PATCH] D29877: Warn about unused static file scope function template declarations.

rsmith added a comment. In https://reviews.llvm.org/D29877#765968, @EricWF wrote: > I think this patch still gets the following case wrong: > > // foo.h > constexpr struct { > template void operator()(T) {} // emits unused template warning > } foo; > What specifically do you think

[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause

rsmith added a comment. In https://reviews.llvm.org/D33538#765146, @EricWF wrote: > In https://reviews.llvm.org/D33538#765045, @rsmith wrote: > > > Do we need to conditionalize this part of libc++? Nothing in the > > header appears to need compiler support. > > > That's correct. I was mistaken

[PATCH] D33568: Fix crash when evaluating constant expressions involving nullptr

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks! https://reviews.llvm.org/D33568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D33538#765062, @rsmith wrote: > In https://reviews.llvm.org/D33538#765045, @rsmith wrote: > > > Do we need to conditionalize this part of libc++? Nothing in the >

[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause

rsmith added a comment. In https://reviews.llvm.org/D33538#765045, @rsmith wrote: > Do we need to conditionalize this part of libc++? Nothing in the > header appears to need compiler support. Oh wait, I see what's going on. You're not testing for whether coroutines is enabled, you're

[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause

rsmith added a comment. Do we need to conditionalize this part of libc++? Nothing in the header appears to need compiler support. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33568: Fix crash when evaluating constant expressions involving nullptr

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:5498-5500 Result.set((Expr*)nullptr, 0, false, true, Offset); +Result.getLValueDesignator() = +SubobjectDesignator(E->getType()->getPointeeType()); This is the only caller

[PATCH] D31692: [coroutines] Wrap the body of the coroutine in try-catch

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D31692 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33339: Fix valid-for-expr ellipses eaten as invalid decl

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D9 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33366: Fix that global delete operator get's assigned to a submodule.

rsmith added a comment. This is supposed to be handled by `Sema::DeclareGlobalAllocationFunction`: DeclContext::lookup_result R = GlobalCtx->lookup(Name); for (DeclContext::lookup_iterator Alloc = R.begin(), AllocEnd = R.end(); Alloc != AllocEnd; ++Alloc) { // Only look at

[PATCH] D33339: Fix valid-for-expr ellipses eaten as invalid decl

rsmith added a comment. In https://reviews.llvm.org/D9#759146, @hubert.reinterpretcast wrote: > The `check-all` target passes even if the ellipsis-after-declarator-id > disambiguation as a declarator is removed entirely. [...] > So, on the whole, the stray ellipsis treatment is both too

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

rsmith added inline comments. Comment at: include/clang/Parse/Parser.h:1956-1957 AccessSpecifier AS, DeclSpecContext DSC); - void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl); + void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl, +

[PATCH] D33339: Fix valid-for-expr ellipses eaten as invalid decl

rsmith added a comment. Should I assume our "misplaced ellipsis" diagnostic requires that we disambiguate the ill-formed ellipsis-after-declarator-id as a declaration in some cases? If so, do we have tests for that somewhere? Comment at: include/clang/Parse/Parser.h:2138 +

[PATCH] D15994: Allow for unfinished #if blocks in preambles.

rsmith accepted this revision. rsmith added a comment. Some comments, but I'm happy for you to go ahead and commit after addressing them. Thanks! Comment at: include/clang/Lex/Preprocessor.h:2004 + ArrayRef getPreambleConditionalStack() const + { return

[PATCH] D33207: Fix an assertion failure in FormatASTNodeDiagnosticArgument.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D33207 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)

rsmith added inline comments. Comment at: lib/Parse/ParseExpr.cpp:203-222 + // Create the ExpressionEvaluationContext on the stack - but only if asked to. + struct EnterExpressionEvaluationContextConditionalRAII { +

[PATCH] D33207: Fix an assertion failure in FormatASTNodeDiagnosticArgument.

rsmith added inline comments. Comment at: test/SemaCXX/warn-shadow.cpp:214 +void handleLinkageSpec() { + typedef void externC; // expected-warning {{declaration shadows a typedef in linkage specification}} +} We should be producing a diagnostic talking about

[PATCH] D32178: Delete unstable integration tests

rsmith added a comment. Clang's regression test suite is not a sensible home for these tests. We should have a home and a plan for system-specific integration tests, but this is not it. Perhaps this should instead be a part of LNT / the test-suite project? https://reviews.llvm.org/D32178

[PATCH] D32332: Add support for transparent overloadable functions in clang

rsmith added a comment. I'd like to suggest an alternative design: don't add a new attribute., and instead change the semantics of `__attribute__((overloadable))` to permit at most one non-overloadable function in an overload set. That one function would implicitly get the

[PATCH] D33013: Driver must return non-zero code on errors in command line

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D33013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D16171: Warning on redeclaring with a conflicting asm label

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. In https://reviews.llvm.org/D16171#540261, @phabricss wrote: > ../include/sys/stat.h:16:15: error: cannot apply asm label to function > after its first use > hidden_proto

[PATCH] D16171: Warning on redeclaring with a conflicting asm label

rsmith added a comment. Er, please ignore the inline review comments; those predated the realisation that this doesn't actually fix the glibc build problem. https://reviews.llvm.org/D16171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33013: Driver must return non-zero code on errors in command line

rsmith added a comment. Thank you, some of these test typos are ... alarming. =) A couple of the test updates don't look quite right, but this mostly looks great. Comment at: test/Driver/amdgpu-features.c:1 -// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri

[PATCH] D32984: [Sema] Implement Core 2094: Trivial copy/move constructor for class with volatile member

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Please first check in a change that just regenerates cxx_dr_status without your changes, to separate out the changes due to the new issues list from the changes due to this patch. (You can go

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

rsmith added inline comments. Comment at: include/clang/Sema/Sema.h:1464 + /// Determine if \p D abd \p Suggested have a structurally compatibale + /// layout as described in C11 6.2.7/1. rsmith wrote: > Typo 'abd' Typo 'compatibale' =)

[PATCH] D32828: [Modules] Fix conservative assertion for import diagnostics

rsmith added inline comments. Comment at: lib/Sema/SemaLookup.cpp:4971-4972 assert(Owner && "definition of hidden declaration is not in a module"); + assert((!isVisible(Decl) || VisibleModules.isVisible(Owner)) && + "missing import for non-hidden decl?");

[PATCH] D29951: Load lazily the template specialization in multi-module setups.

rsmith added inline comments. Comment at: lib/Serialization/ASTReaderDecl.cpp:213-215 + assert(isa(D) || + isa(D) && + "Decl doesn't have specializations."); Can this ever fail at runtime? I'd expect the below code to not compile if

[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This makes a lot of sense to me. See also r302463: I think we probably want three levels of shadowing here: the main input shadows -fmodule-map-file, which shadows module maps loaded

[PATCH] D32499: Further delay calling DeclMustBeEmitted until it's safe.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Let's go ahead with this. I've been unable to find a testcase that triggers the problem, but we shouldn't keep a known latent bug around just because we don't know how to expose it yet.

[PATCH] D32984: [Sema] Implement Core 2094: Trivial copy/move constructor for class with volatile member

rsmith added inline comments. Comment at: test/CXX/drs/dr4xx.cpp:1205 -namespace dr496 { // dr496: no +namespace dr496 { // dr496: reverted by dr2095 in 5.0 struct A { int n; }; Write this as "dr496: sup 2094" and then rerun the `make_cxx_dr_status` script

[PATCH] D32372: Arrays of unknown bound in constant expressions

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Committed as r301822. Comment at: lib/AST/ExprConstant.cpp:1301 void addUnsizedArray(EvalInfo , QualType ElemTy) { - assert(Designator.Entries.empty() &&

[PATCH] D32675: in expression evaluator, treat non-literal types as discarded value expressions if EvalInfo says to continue evaluating them

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks. Could you also add something like: struct A {}; struct B : virtual A {}; constexpr A = (A&)*(B*)0; to test/SemaCXX/constant-expression-cxx11.cpp to ensure we produce a suitable

[PATCH] D32372: Arrays of unknown bound in constant expressions

rsmith added a comment. Thanks, this looks good, just a couple of minor things and then it should be ready to land. Do you have commit access or will you need someone else to commit this for you? Comment at: lib/AST/ExprConstant.cpp:152 + uint64_t ,

[PATCH] D32566: Revert rL301328 and add tests for the regressions introduced.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Yes, let's first revert back to the clang 4.0 behavior, then please mail cfe-dev to discuss what the right behavior should be. Repository: rL LLVM https://reviews.llvm.org/D32566

[PATCH] D32410: change the way the expr evaluator handles objcboxedexpr

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D32410 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30427: Fix whitespace before token-paste of an argument.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM Comment at: test/Preprocessor/macro_paste_commaext.c:4 +// In the following tests, note that the output is sensitive to the +// whitespace *preceeding* the varargs

[PATCH] D32566: Revert rL301328 and add tests for the regressions introduced.

rsmith added a comment. I'm OK with this from a mechanical perspective. But there's also a libclang design question here: what should the libclang methods to query template arguments for a type cursor representing an alias template specialization actually do? Should there be some way for a

[PATCH] D32405: Expr evaluator may want to continue into ArraySubscriptExpr if evaluation mode indicates

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D32405 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32410: change the way the expr evaluator handles objcboxedexpr

rsmith added inline comments. Comment at: lib/AST/ExprConstant.cpp:4469-4470 { return StmtVisitorTy::Visit(E->getSubExpr()); } + bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E) +{ return StmtVisitorTy::Visit(E->getSubExpr()); } bool VisitChooseExpr(const ChooseExpr

[PATCH] D32412: analyze all kinds of expressions for integer overflow

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. If we're now catching integer overflow in more cases, please add some relevant testcases. If this is a pure refactoring that enables those additional diagnostics to be produced in future,

[PATCH] D32455: detect integer overflow inside arms of conditional operator with non-constant expression

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D32455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32348: [libclang] Check for a record declaration before a template specialization.

rsmith added a comment. This change looks like it introduces a regression itself: given template struct A {}; template using B = A; B bi; ... requesting the template arguments for the type `B` changes from producing `int` to producing `int*` with this patch, which seems to directly

[PATCH] D32372: Arrays of unknown bound in constant expressions

rsmith added a comment. The change in direction from diagnosing the lvalue-to-rvalue conversion to diagnosing the pointer arithmetic seems fine to me (and is likely a better approach overall), but this means we should now treat a designator referring to element 0 of an array of unknown /

[PATCH] D31588: Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)

rsmith added inline comments. Comment at: lib/Parse/ParseTemplate.cpp:1203-1204 + { +EnterExpressionEvaluationContext EnterConstantEvaluated( +Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated); +if (isCXXTypeId(TypeIdAsTemplateArgument)) {

[PATCH] D28832: Improve redefinition errors pointing to the same header.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM. I'd like to make sure we try to use something better than the first import location for a module (that location is especially meaningless under `-fmodules-local-submodule-visibility`),

[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons

rsmith added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:3066-3067 +} else { // Unsigned integers and pointers. + if (CGF.CGM.getCodeGenOpts().StrictVTablePointers && + CGF.CGM.getCodeGenOpts().OptimizationLevel > 0) { +// Based on

[PATCH] D31856: Headers: Make the type of SIZE_MAX the same as size_t

rsmith added a comment. In https://reviews.llvm.org/D31856#733845, @efriedma wrote: > We normally use stdint.h from the host C library, rather than our own > version; this file is only relevant in -ffreestanding mode. So it should be > safe to change. Agreed; r89237 (and nearby changes:

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

rsmith added inline comments. Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:46 +std::gcd(static_cast(0), static_cast(0)))>::value, ""); +const bool result = static_cast>(out) == +

[PATCH] D32372: Arrays of unknown bound in constant expressions

rsmith added a comment. This needs testcases (the one from your summary plus the ones in my comments above would be good). Comment at: lib/AST/ExprConstant.cpp:2622 // Next subobject is an array element. - const ConstantArrayType *CAT =

[PATCH] D32269: [Driver] Add iSOFTLinux to GNU ToolChains X86Triple

rsmith added a comment. Please add some test coverage for these triples. Repository: rL LLVM https://reviews.llvm.org/D32269 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31856: Headers: Make the type of SIZE_MAX the same as size_t

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. This is sadly not a correct change. The relevant requirements (C11 7.20.3/2) on these macros are: > Each instance of these macros shall be replaced by a constant expression >

[PATCH] D32265: Add __CLANG_ATOMIC__LOCK_FREE macros for use in MSVC compatibility mode.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. I'm not thrilled about adding yet more predefined macros, but it really doesn't make sense for libc++ to depend on `__GCC_*` macros when targeting Windows, nor for these to be Windows-only,

[PATCH] D32199: [TBAASan] A TBAA Sanitizer (Clang)

rsmith added a comment. In https://reviews.llvm.org/D32199#732189, @hfinkel wrote: > In https://reviews.llvm.org/D32199#731472, @rsmith wrote: > > > 1. C's "effective type" rule allows writes to set the type pretty much > > unconditionally, unless the storage is for a variable with a declared

[PATCH] D32199: [TBAASan] A TBAA Sanitizer (Clang)

rsmith added a comment. > As I've currently implemented it, both reads and writes set the type of > previously-unknown storage, and after that it says fixed (unless you memcpy > to it, memset it, or its lifetime ends (the type gets reset on > lifetime.start/end and for malloc/allocas/etc.).

[PATCH] D32251: Implement DR1601 - Promotion of enumeration with fixed underlying type

rsmith added inline comments. Comment at: lib/Sema/SemaOverload.cpp:3850-3853 // In Microsoft mode, prefer an integral conversion to a // floating-to-integral conversion if the integral conversion // is between types of the same size. // For example:

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

rsmith added inline comments. Comment at: include/clang/Sema/Sema.h:1464 + /// Determine if \p D abd \p Suggested have a structurally compatibale + /// layout as described in C11 6.2.7/1. Typo 'abd' Comment at:

[PATCH] D32199: [TBAASan] A TBAA Sanitizer (Clang)

rsmith added a comment. > ! In https://reviews.llvm.org/D32199#731252, @hfinkel wrote: > >> How about renaming this to something more like `-fsanitize=type`? > > I'm fine with that. Do you like TypeSanitizer or TypeAccessSantizer or > TypeAliasingSanitizer best? I think calling it a type

[PATCH] D32199: [TBAASan] A TBAA Sanitizer (Clang)

rsmith added a comment. I don't like calling this a "TBAA sanitizer". What we're sanitizing is the object model and effective type rules; it seems irrelevant which specific compiler analysis passes would result in your program misbehaving if you break the rules. I would also expect that we

[PATCH] D32092: Attribute inline

rsmith added a comment. From some very superficial testing, it looks like CL treats `__declspec(inline)` exactly like a synonym for `inline` (it even rejects them both appearing on the same declaration, complaining about a duplicate `inline` specifier). So modeling this as `GNUInlineAttr` is

[PATCH] D32092: Attribute inline

rsmith added a comment. In https://reviews.llvm.org/D32092#727543, @zahiraam wrote: > Pushed the submit too fast ... > Before I submitted this review, I have done some experiments and inline and > declspec(inline) have the same behavior. Compiling with /Ob0 disables > inlining. With -O1 or

[PATCH] D32092: Attribute inline

rsmith added inline comments. Comment at: include/clang/Basic/Attr.td:869 def GNUInline : InheritableAttr { - let Spellings = [GCC<"gnu_inline">]; + let Spellings = [GCC<"gnu_inline">, Declspec<"inline">]; let Subjects = SubjectList<[Function]>;

[PATCH] D27263: Address of bitfield in anonymous struct doesn't error.

rsmith added a comment. The change to test/SemaCXX/anonymous-struct.cpp appeared to be unrelated to the rest of the patch, so I committed it separately as r300266. Thank you! Repository: rL LLVM https://reviews.llvm.org/D27263 ___ cfe-commits

[PATCH] D27263: Address of bitfield in anonymous struct doesn't error.

This revision was automatically updated to reflect the committed changes. Closed by commit rL300264: Diagnose attempt to take address of bitfield members in anonymous structs. (authored by rsmith). Changed prior to commit: https://reviews.llvm.org/D27263?vs=79759=95222#toc Repository: rL

[PATCH] D27546: [ASTReader] Sort RawComments before merging

rsmith added inline comments. Comment at: lib/Serialization/ASTReader.cpp:8487 +std::sort(Comments.begin(), Comments.end(), + BeforeThanCompare(SourceMgr)); Context.Comments.addDeserializedComments(Comments); Does this cause us to

[PATCH] D27604: [Driver] Add compiler option to generate a reproducer

rsmith accepted this revision. rsmith added a comment. LGTM with one change. Comment at: include/clang/Basic/DiagnosticDriverKinds.td:95 def err_drv_force_crash : Error< - "failing because environment variable '%0' is set">; + "failing because %select{environment

[PATCH] D31781: [Modules] Allow local submodule visibility without c++

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Can you also add a basic test that this works in C? Thanks! https://reviews.llvm.org/D31781 ___ cfe-commits mailing list

[PATCH] D29877: Warn about unused static file scope function template declarations.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM with the overloaded operator check removed. https://reviews.llvm.org/D29877 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29877: Warn about unused static file scope function template declarations.

rsmith added inline comments. Comment at: lib/Sema/Sema.cpp:473 if (const FunctionDecl *FD = dyn_cast(D)) { +// If this is a function template and neither of its specs is used, warn. +if (FunctionTemplateDecl *Template = FD->getDescribedFunctionTemplate())

[PATCH] D31187: Fix removal of out-of-line definitions.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. The patch itself LGTM. I'd like some test coverage, but if this will be covered by some upcoming interpreter piece and you need this to unblock yourselves, that's good enough for me. In any

[PATCH] D31069: Don't warn about an unreachable fallthrough annotation in a template function

This revision was automatically updated to reflect the committed changes. Closed by commit rL298477: Suppress warning on unreachable [[clang::fallthrough]] within a template… (authored by rsmith). Changed prior to commit: https://reviews.llvm.org/D31069?vs=92104=92588#toc Repository: rL

[PATCH] D30954: Modules: Simulate diagnostic settings for implicit modules

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM > - The patch-as-is checks whether pragmas should be demoted to warnings for > all AST files, not just implicit modules. I can add a bit of logic to > `ReadPragmaDiagnosticMappings`

[PATCH] D30848: Implement DR 373 "Lookup on namespace qualified name in using-directive"

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, LGTM https://reviews.llvm.org/D30848 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31069: Don't warn about an unreachable fallthrough annotation in a template function

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, do you need someone to commit for you? https://reviews.llvm.org/D31069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31069: Don't warn about an unreachable fallthrough annotation in a template function

rsmith added a comment. This needs a test case, but the change itself looks fine to me. Repository: rL LLVM https://reviews.llvm.org/D31069 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29877: Warn about unused static file scope function template declarations.

rsmith added inline comments. Comment at: lib/Sema/Sema.cpp:472-477 +// If this is a function template, we should remove if it has no +// specializations. +if (FunctionTemplateDecl *Template = FD->getDescribedFunctionTemplate()) { + if

[PATCH] D30848: Implement DR 373 "Lookup on namespace qualified name in using-directive"

rsmith added a comment. Functionally, this looks good. How do the diagnostics look in the case where lookup only finds a non-namespace name? Eg, struct A { struct B {}; }; namespace X = A::B; https://reviews.llvm.org/D30848 ___ cfe-commits

[PATCH] D30848: Implement DR 373 "Lookup on namespace qualified name in using-directive"

rsmith added inline comments. Comment at: test/CXX/drs/dr3xx.cpp:911 -namespace dr373 { // dr373: no - // FIXME: This is valid. - namespace X { int dr373; } // expected-note 2{{here}} +namespace dr373 { // dr373: yes + namespace X { int dr373; } This should

[PATCH] D26057: [coroutines] Add DependentCoawaitExpr and fix re-building CoroutineBodyStmt.

rsmith added inline comments. Comment at: lib/Sema/TreeTransform.h:6802 + return getDerived().RebuildDependentCoawaitExpr( + E->getKeywordLoc(), Result.get(), E->getOperatorCoawaitLookup()); +} EricWF wrote: > rsmith wrote: > > You need to transform the

[PATCH] D26057: [coroutines] Add DependentCoawaitExpr and fix re-building CoroutineBodyStmt.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/Sema/ScopeInfo.h:138-140 + /// \brief Whether this function has already built, or tried to build, the + /// the initial and final coroutine

[PATCH] D30590: Don't assume cleanup emission preserves dominance in expr evaluation

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Hal and I discussed exactly the same problem (in the context of coroutines) on Saturday and came up with exactly the same approach :) I think this is the right direction.

[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

rsmith added a comment. In https://reviews.llvm.org/D29753#688834, @bruno wrote: > > It seems to me that the problem here is that `DeclMustBeEmitted` is not > > safe to call in the middle of deserialization if anything > > partially-deserialized might be reachable from the declaration we're >

[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

rsmith accepted this revision. rsmith added a comment. Yes, I'm OK with this as a stopgap fix for 4.0. I would have preferred a more full fix for 4.0 but we've run out of time for that, and the PCH case does seem more pressing. https://reviews.llvm.org/D29753

[PATCH] D21626: Lit C++11 Compatibility Patch #10

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM with a couple of changes. Comment at: test/Modules/Inputs/merge-using-decls/a.h:25 +#if __cplusplus <= 199711L // C++11 does not allow access declerations template

[PATCH] D20710: Lit C++11 Compatibility Patch #9

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D20710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29812: Update template-id-expr.cpp test to work when compiler defaults to non-C++03 standard

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D29812 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30315: [Driver] Move architecture-specific free helper functions to their own files.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. It might make sense to move the *Arch.cpp files to a subdirectory of lib/Driver, but otherwise this looks good. https://reviews.llvm.org/D30315

[PATCH] D30082: Fix assertion when generating debug information for deduced template specialization types.

This revision was automatically updated to reflect the committed changes. Closed by commit rL295794: Fix assertion failure when generating debug information for a variable (authored by rsmith). Changed prior to commit: https://reviews.llvm.org/D30082?vs=88943=89300#toc Repository: rL LLVM

[PATCH] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Serialization/ASTBitCodes.h:258 + /// A block containing unhashed contents. It currently holds Diagnostic + /// Options and

[PATCH] D30082: Fix assertion when generating debug information for deduced template specialization types.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D30082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30082: Fix assertion when generating debug information for deduced template specialization types.

rsmith added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:2478 break; +case Type::DeducedTemplateSpecialization: { + QualType DT = cast(T)->getDeducedType(); EricWF wrote: > I'll put this in alphabetical order before committing.

[PATCH] D29863: [libc++] Fix PR 31938 - std::basic_string constructors use non-deductible parameter types.

rsmith added a comment. Other than (5), all the failing cases look like they should fail per the current `basic_string` spec. Comment at: test/std/strings/basic.string/string.cons/implicit_deduction_guides.pass.cpp:57 + { // Testing (2) +// FIXME: (2) doesn't work with

[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

rsmith added a comment. It seems to me that the problem here is that `DeclMustBeEmitted` is not safe to call in the middle of deserialization if anything partially-deserialized might be reachable from the declaration we're querying, and yet we're currently calling it in that case. I don't see

[PATCH] D28510: Reinstate CWG1607 restrictions on lambdas appearing inside certain constant-expressions

rsmith added a comment. In https://reviews.llvm.org/D28510#654821, @faisalv wrote: > In https://reviews.llvm.org/D28510#653794, @rsmith wrote: > > > I don't think it's possible to check this in the way you're doing so here. > > In general, there's no way to know whether a constant expression

[PATCH] D29724: [Driver] Report available language standards on user error

rsmith added a comment. Committed as r295113. https://reviews.llvm.org/D29724 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29930: Add `__is_direct_constructible` trait for checking safe reference initialization.

rsmith added a comment. I don't like this name; it sounds too much like you're asking whether a certain direct-initialization is possible, which is what `__is_constructible` does. I also don't like the idea of combining an "is this type direct-initializable from this list of arguments" check

[PATCH] D29748: [cxx1z-constexpr-lambda] Implement captures - thus completing implementation of constexpr lambdas.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/AST/ExprConstant.cpp:428-429 +llvm::DenseMap LambdaCaptureFields; +FieldDecl *LambdaThisCaptureField; + I'm a little concerned

[PATCH] D29863: [libc++] Fix PR 31938 - std::basic_string constructors use non-deductible parameter types.

rsmith added inline comments. Comment at: include/string:782 _LIBCPP_INLINE_VISIBILITY basic_string(const value_type* __s, size_type __n); _LIBCPP_INLINE_VISIBILITY EricWF wrote: > rsmith wrote: > > Did you skip this one intentionally? > Yes.

[PATCH] D29724: [Driver] Report available language standards on user error

rsmith added a comment. LGTM, do you need someone to commit for you? https://reviews.llvm.org/D29724 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

  1   2   >