[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 of `set()` that passes more than three arguments (that is, the only caller that passes `true` as `IsNullPtr_`). It seems that such calls would always be unsafe / wrong, so I think we can do better than this. How about this: split `set()` into two functions: for one of them, remove the last two parameters (`IsNullPtr_` and `Offset_`), and for the other one, rename to `setNull()` and just pass a `QualType` and offset. https://reviews.llvm.org/D33568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 testing for whether the `__builtin_coro_*` builtins exist. Are we sufficiently confident that those aren't going to change that we're prepared to make libc++ rely on this? (If we change the signature of those builtins in the future, then new versions of clang would stop being able to build old versions of the libc++ module.) If we're not confident of that, how about calling the new feature something ugly like experimental_coroutines_builtins_20170525 or similar? That way, future versions of Clang can stop advertising that feature if we change the design of the builtins, and we can add a feature 'coroutines' later in a non-disruptive way if/when we decide we're happy with them as-is. Comment at: test/Modules/requires-coroutines.mm:1 +// RUN: rm -rf %t +// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F %S/Inputs %s -verify EricWF wrote: > Should this test be called `requires-coroutines.cpp` or is using Obj-C++ a > correct thing to do here? You can use a .cpp extension and the Modules TS `import` keyword if you prefer (add `-fmodules-ts` to the clang arguments), or use a .cpp extension and `#pragma clang module import` if you don't want to depend on a second TS in this test. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 > > 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 testing for whether the `__builtin_coro_*` builtins exist. > Are we sufficiently confident that those aren't going to change that we're > prepared to make libc++ rely on this? (If we change the signature of those > builtins in the future, then new versions of clang would stop being able to > build old versions of the libc++ module.) On reflection, I think this is fine. If the signatures of the builtins change, and the user builds with `-fmodules` `-fcoroutines-ts` and libc++, and there's version skew between libc++ and clang, they'll get a compile error. That's mostly the expected outcome; the issue is that we'd produce this compile error *even if* they never `#include `, because building the complete libc++ module would fail in that situation. If we're worried about that, we could split the coroutines header out of the main libc++ module into its own top-level module, but I don't think we need to worry too much about rejecting code that uses `-fcoroutines-ts` but never actually uses a coroutine. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 as to why this was needed. I mistook a bug in > libc++ for the reason this was needed. > So I have no need for this patch anymore. > > Do you still want to land this for the reasons you mentioned? r303936 will break the libc++ modules build if used with an older version of clang that doesn't have the coroutines builtins. If you're OK with that, then we don't need this. But if you intend to support older versions of Clang, then I think you need either this or a different approach (such as splitting out a separate top-level module for the coroutines header) to avoid that problem. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 is wrong here? There is an unused internal
linkage function template here. If we want to warn on unused internal linkage
templates declared in headers, we should warn on this one.
Note that any use of `foo` here from an inline function would result in ODR
violations (because you get a different `foo` in each translation unit), so
it's probably at least a bad idea to do that. We could suppress this warning
for unused internal linkage templates declared in headers, or maybe move that
to a separate warning flag; can you point us at some code that does this in
practice and isn't wrong?
https://reviews.llvm.org/D29877
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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: https://github.com/ericniebler/range-v3/blob/a4829172c0d6c43687ba213c54f430202efd7497/include/range/v3/view/zip_with.hpp#L44 This code is wrong, and creates ODR violations on lines 190 and 200. It seems to me that the warning is firing on dangerous / broken code (yay!) but the warning is not sufficient to explain *why* the code is broken (boo!). It also seems to me that the reason why we flag up this code is not really related to the reason why this code is broken, and we should probably have a separate diagnostic for using an internal linkage entity from within an entity to which the ODR applies that is defined within a header. If we had such a diagnostic, it would seem reasonable to limit this warning to only apply to code that is *not* in a header file -- but see PR22712 for a case where even that is not quite enough. https://reviews.llvm.org/D29877 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 required to be contextually convertible to 'bool'" I would drop the leading 'the' from both of these diagnostics for consistency with our normal terse sentence fragment style. Comment at: lib/Sema/SemaCoroutine.cpp:393 +// - await-suspend is the expression e.await_suspend(h), which shall be +// a prvalue of type void or bool. +QualType RetType = AwaitSuspend->getType(); It looks like you're not checking the 'prvalue' part of this. https://reviews.llvm.org/D33625 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC
rsmith added a comment.
Other options:
Do we need to perfectly preserve the functionality of `#pragma once` /
`#import`? That is, would it be acceptable to you if we spuriously enter files
that have been imported in that way, while building a module? If we view them
as pure optimization, we can do something //substantially// simpler here, for
instance by never importing "isImported" flags from modules and throwing away
all our "isImported" flags whenever module visibility decreases.
Another possibility -- albeit slightly hacky -- would be to invent a
controlling macro if the header in question turns out to not have one (say,
form an identifier from the path by which the file was included and use that as
the macro name) and is #imported / uses #pragma once. Or we could try simply
rejecting textual headers that are #import'd / #pragma once'd and have no
controlling macro in a modules build.
Comment at: include/clang/Lex/HeaderSearch.h:98-101
+ /// List of modules that import from this header. Since the number of modules
+ /// using the same FileEntry is usually small (2 or 3 at most, often related
+ /// to builtins) this should be enough. Choose a more appropriate data
+ /// structure in case the requirement changes.
I don't think this is true in general. For a textual header, there could be
hundreds or thousands of loaded modules that use it. If all header files in a
project start with
#include "local_config.h"
... which is configured to be a textual header for whatever reason and contains
a `#pragma once`, we would get into that situation for at least that one
header. While a vector might be OK (as the number of entries should at least
grow only linearly with the size of the entire codebase), quadratic-time
algorithms over it probably won't be. Perhaps a sorted vector instead?
Comment at: lib/Lex/HeaderSearch.cpp:1116-1119
+// - if there not associated module or the module already imports
+//from this header.
+// - if any module associated with this header is visible.
+if (FileInfo.isImport) {
I think this would be clearer as:
> [...], ignore it, unless doing so will lead to us importing a module that
> contains the file but didn't actually include it (because we're still
> building the corresponding module), and we've not already made the file
> visible by importing some other module.
... but I don't think that's actually right. If `M` is null (if the file is
only ever included as a textual header), we still need to check whether some
visible module has actually made it visible, and we should enter it if not.
Comment at: lib/Serialization/ASTReader.cpp:1691-1692
HFI.isModuleHeader |= !(HeaderRole & ModuleMap::TextualHeader);
+if (HFI.isImport)
+ HFI.addModulesUsingHdr(Mod);
}
Doing this here will do the wrong thing while building a module with local
submodule visibility enabled -- we need to know whether the visible module set
contains a module that entered the header, even for modules that we've never
serialized.
Also, this will not populate the vector for the cases where the header was not
listed in the module map, but was simply a non-moduler header that was
textually entered while building a module.
https://reviews.llvm.org/D26267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27784: Add a class ASTRecordReader which wraps an ASTReader, a RecordData, and ModuleFile.
rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Serialization/ASTReader.cpp:5870-5876 +TL.setWrittenTypeSpec( +static_cast(Reader.getRecordData()[Idx++])); +TL.setWrittenSignSpec( +static_cast(Reader.getRecordData()[Idx++])); +TL.setWrittenWidthSpec( +static_cast(Reader.getRecordData()[Idx++])); +TL.setModeAttr(Reader.getRecordData()[Idx++]); Can you remove the `.getRecordData()` here and use `ASTRecordReader::operator[]` instead? https://reviews.llvm.org/D27784 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27836: Store the "current position" index within the ASTRecordReader.
rsmith added a comment. I like moving `Idx` into the reader, but I have mixed feelings about the iterator-like interface. For consistency with the rest of the `ASTRecordReader` interface, and with how we model the writer side, I think perhaps `Record.ReadInt()` would fit better than using `*Record++`. https://reviews.llvm.org/D27836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24969: [Sema] Use the instantiated name of destructors in FindInstantiatedDecl and RebuildMemberExpr
rsmith added a comment. Looks good, other than error recovery. Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:4851 + DeclarationNameInfo NameInfo(Name, D->getLocation()); + Name = SubstDeclarationNameInfo(NameInfo, TemplateArgs).getName(); + DeclContext::lookup_result Found = ParentDC->lookup(Name); You should deal with the possibility of `SubstDeclarationNameInfo` failing (which will happen if substitution into the name creates an invalid type). If you get a null name back from the `Subst` call, just return null. Comment at: lib/Sema/TreeTransform.h:8766-8767 NamedDecl *FirstQualifierInScope = nullptr; + DeclarationNameInfo MemberNameInfo = + getDerived().TransformDeclarationNameInfo(E->getMemberNameInfo()); Likewise here, you should return `ExprError()` if the transformed name is null (please also add an assert that the name is not null before the transform, as in that case a null transformed name would not indicate an error has been diagnosed). https://reviews.llvm.org/D24969 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27836: Store the "current position" index within the ASTRecordReader.
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, any chance I can tempt you to lowerCamelCase all the other ASTRecordReader members to match the new ones as a follow-up change? https://reviews.llvm.org/D27836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26882: Refactor how FunctionDecl handles constexpr:
rsmith added inline comments. Comment at: include/clang/AST/Decl.h:1923 +IsConstexpr = IC; +setImplicitlyInline(); + } nwilson wrote: > hubert.reinterpretcast wrote: > > I am quite sure this is not the right thing to do when `IC` is `false`. > Good point. I //could// do `if (IC) setImplicitlyInline();`, but that doesn't > seem great with these smaller functions. Any suggestions by you or @rsmith > would be great! Doing something automatic here with the inline specifier is turning out to be too complex. Instead, let's just remove the `setImplicitlyInline()` call from here and call it explicitly when handling a concept. https://reviews.llvm.org/D26882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28148: [Sema] Suppress warnings for C's zero initializer
rsmith added a comment. Why are you adding a language option for this? Just use `!LangOpts.CPlusPlus`. https://reviews.llvm.org/D28148 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28148: [Sema] Suppress warnings for C's zero initializer
rsmith added inline comments.
Comment at: lib/Sema/SemaInit.cpp:884
+bool MissingBracesOkay = false;
+
+if (!SemaRef.getLangOpts().CPlusPlus &&
Remove empty line.
Comment at: lib/Sema/SemaInit.cpp:885-892
+if (!SemaRef.getLangOpts().CPlusPlus &&
+StructuredSubobjectInitList->getNumInits() == 1) {
+ if (const IntegerLiteral *lit =
dyn_cast(StructuredSubobjectInitList->getInit(0))) {
+if (lit->getValue() == 0) {
+ MissingBracesOkay = true;
+}
+ }
I think it would make more sense to check `ParentIList` here instead of
`StructuredSubobjectInitList` -- we want to check whether the list the user
wrote in the source code was `{0}`, not the list after semantic checking. This
would matter for a case like:
```
struct A { int n; };
struct B { struct A a; };
struct C { struct B b; };
C c = {0};
```
`ParentIList` will be `{0}` at every level here, but it looks like
`StructuredSubobjectInitList` will be `{{0}}` when checking the initialization
of `c.b`, so you won't suppress the warning.
It would also matter for a case like
```
struct A { short p; };
struct B { A a; };
B b = {0};
```
where the list after semantic checking will have an implicit conversion wrapped
around the initializer.
Comment at: lib/Sema/SemaInit.cpp:1843-1851
+ // Check if this is C's zero initializer { 0 }
+ if (!SemaRef.getLangOpts().CPlusPlus &&
+ IList->getNumInits() == 1) {
+if (const IntegerLiteral *lit =
dyn_cast(IList->getInit(0))) {
+ if (lit->getValue() == 0) {
+CheckForMissingFields = false;
+ }
Please factor this check out into something like
`InitListExpr::isIdiomaticZeroInitializer()`. It would make sense for that
function to also assert `isSyntactic()`.
https://reviews.llvm.org/D28148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28166: Properly merge K&R functions that have attributes
rsmith added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:7464-7470
+const Type *NonAttributedFTy = R.getTypePtr();
+while (const auto *AttrTy = NonAttributedFTy->getAs()) {
+ NonAttributedFTy = AttrTy->getModifiedType().getTypePtr();
+}
bool HasPrototype =
(D.isFunctionDeclarator() && D.getFunctionTypeInfo().hasPrototype) ||
+ (!isa(NonAttributedFTy) && R->isFunctionProtoType());
Rather than hardcoding the forms of type sugar that can appear here, can we
just use `R.getTypePtr()->getAs()`? I expect we can also have
`ParenType`s wrapping the `FunctionNoProtoType` (`int (f)();`).
Comment at: lib/Sema/SemaDecl.cpp:11958-11962
+// The type location may be attributed; strip the attributes to get to
+// the function type location.
+while (auto ATL = TL.getAs()) {
+ TL = ATL.getModifiedLoc();
+}
Again, I don't like having this knowledge about what kinds of type sugar can
appear in a function declaration hardcoded here. Please put this somewhere more
central.
A quick look finds that `FunctionDecl::getReturnTypeSourceRange()` gets this
wrong in the opposite direction: it skips parens but not attributes. Maybe we
should have a `TypeLoc::getAsAdjusted` or similar, that walks over type
sugar nodes that represent some kind of type adjustment from a type that was
written as a T to another type that is still canonically a T (`ParenType`,
`AttributedType`, `ElaboratedType`).
https://reviews.llvm.org/D28166
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28148: [Sema] Suppress warnings for C's zero initializer
rsmith added a comment.
Looks good to go once Daniel's and my comments are addressed. Thank you.
Comment at: lib/AST/Expr.cpp:1887
+bool InitListExpr::isIdiomaticZeroInitializer(const LangOptions &LangOpts)
const {
+ assert(!getSyntacticForm() && "only test syntactic form as zero
initializer");
+
`!isSyntacticForm()` would be preferable here instead of `!getSyntacticForm()`.
https://reviews.llvm.org/D28148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28207: Add second fast path for DiagnosticsEngine::GetDiagStatePointForLoc
rsmith added inline comments. Comment at: lib/Basic/Diagnostic.cpp:179 + + // 2nd most frequent case: L is before the first diag state change. + FullSourceLoc FirstStateChangePos = DiagStatePoints[1].Loc; It's surprising to me that this would be particularly frequent. I suspect what you're actually seeing is a consequence of a bug in how we manage `DiagStatePoint`s with modules. It looks like `ASTReader::InitializeContext` is called once per top-level PCM file that we load, and its call to `ReadPragmaDiagnosticMappings` adds entries to the `DiagStatePoints` list regardless of whether they've already been added. So, we'll end up with duplicates in the `DiagStatePoints` list, and it won't even be in translation unit order. Can you take a look at the `DiagStatePoints` list for a translation unit where you see a performance problem and check whether it seems reasonable? https://reviews.llvm.org/D28207 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28166: Properly merge K&R functions that have attributes
rsmith added a comment.
The test failure in test/CodeGen/microsoft-call-conv-x64.c definitely indicates
a problem. The code has defined behavior, but the IR you say we now produce has
undefined behavior due to a type mismatch between the call and the callee.
It looks to me like unprototyped `__stdcall` lowering is broken (prior to your
change). Consider:
void __stdcall g(int n) {}
void __stdcall (*p)() = g;
void f() { p(0); }
The types of `p` and `g` are compatible (`g`'s parameter type list does not end
in an ellipsis and its parameter type `int` is a promoted type, so it is
compatible with an unprototyped function), so the above program is valid, and a
call to `f` has defined behavior.
And yet we lower the definition of `g` to `define void @g(i32 %n) ` and the
call to
%0 = load void (...)*, void (...)** @p, align 8
%callee.knr.cast = bitcast void (...)* %0 to void (i64)*
call void %callee.knr.cast(i64 0)
... resulting in undefined behavior.
https://reviews.llvm.org/D28166
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28166: Properly merge K&R functions that have attributes
rsmith added a comment. In https://reviews.llvm.org/D28166#633621, @aaron.ballman wrote: > Do you think this patch should be gated on (or perhaps combined with) a fix > for the lowering bug, or do you think this patch is reasonable on its own? > Given that it turns working code into UB, I think my preference is to gate it > on a fix for the lowering bug, but I'm also not certain I am the right person > to implement that fix (though I could give it a shot). The test in question has a comment pointing to PR7117, which in turn indicates that we might miscompile parts of FreeBSD if we land this as-is. So I think we need to gate this on a fix for the lowering bug. https://reviews.llvm.org/D28166 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28222: [libcxx] Re-implement LWG 2770 again: Fix tuple_size to work with structured bindings
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
LGTM, but a minor simplification seems possible.
Comment at: include/__tuple:32
template
-class _LIBCPP_TYPE_VIS_ONLY tuple_size
-: public __tuple_size_base_type<_Tp>::type {};
+class _LIBCPP_TYPE_VIS_ONLY tuple_size<__enable_if_tuple_size_imp::type>::value)>>
+: public integral_constant::value> {};
remove_cv looks redundant here, deduction already stripped the cv qualifiers.
https://reviews.llvm.org/D28222
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28166: Properly merge K&R functions that have attributes
rsmith added a comment. In https://reviews.llvm.org/D28166#634196, @aaron.ballman wrote: > So I think the correct behavior is to only enable the vararg behavior when > the function is variadic with an ellipsis rather than variadic due to a lack > of prototype. That sounds right. Note that functions with no prototype are not actually variadic, so it's permitted for a vararg definition to use a different calling convention from a no-prototype call, as happens in this case. https://reviews.llvm.org/D28166 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28258: [Sema] Handle invalid noexcept expressions correctly.
rsmith added inline comments.
Comment at: lib/Parse/ParseDeclCXX.cpp:3547
NoexceptRange = SourceRange(KeywordLoc, T.getCloseLocation());
-} else {
- NoexceptType = EST_None;
}
} else {
Should `NoexceptRange` be set in the `else` case too, now that we're claiming
that the type is `EST_ComputedNoexcept`?
Comment at: lib/Sema/TreeTransform.h:5044-5057
+if (!NoexceptExpr.isUsable())
return true;
// FIXME: This is bogus, a noexcept expression is not a condition.
NoexceptExpr = getSema().CheckBooleanCondition(Loc, NoexceptExpr.get());
-if (NoexceptExpr.isInvalid())
+if (!NoexceptExpr.isUsable())
return true;
These changes don't make sense to me: if we get a valid-but-null `ExprResult`
from any of the above, there is no guarantee a diagnostic has been produced, so
it is not correct to return `true`.
Which call is producing the valid-but-null `ExprResult`?
https://reviews.llvm.org/D28258
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28207: Add second fast path for DiagnosticsEngine::GetDiagStatePointForLoc
rsmith added inline comments.
Comment at: lib/Basic/Diagnostic.cpp:179
+
+ // 2nd most frequent case: L is before the first diag state change.
+ FullSourceLoc FirstStateChangePos = DiagStatePoints[1].Loc;
djasper wrote:
> rsmith wrote:
> > It's surprising to me that this would be particularly frequent.
> >
> > I suspect what you're actually seeing is a consequence of a bug in how we
> > manage `DiagStatePoint`s with modules. It looks like
> > `ASTReader::InitializeContext` is called once per top-level PCM file that
> > we load, and its call to `ReadPragmaDiagnosticMappings` adds entries to the
> > `DiagStatePoints` list regardless of whether they've already been added.
> > So, we'll end up with duplicates in the `DiagStatePoints` list, and it
> > won't even be in translation unit order.
> >
> > Can you take a look at the `DiagStatePoints` list for a translation unit
> > where you see a performance problem and check whether it seems reasonable?
> I looked at the DiagStatePoints and they do look somewhat sane but suspicious.
>
> The translation unit I am looking at has (AFAICT) only includes that get
> translated to modules. DiagStatePoints only has entries for the translation
> unit itself, not a single one coming from any of the modules. So
> DiagStatePoints looks like:
>
> [0]: <> (for the command line)
> [1]: myfile.cc:100
> [2]: myfile.cc:100
> ...
> [2500]: myfile.cc:2800
>
> And because of that, the new fast path is hit every single time when a source
> location coming from a module is queried. There are always two entries for a
> line of myfile.cc which always seem to denote entering and exiting a macro.
>
> So, specific questions:
> - Should there by DiagStatePoints from modules?
> - Should there really be a DiagStatePoint entry (or actually two) for every
> macro invocation in myfile.cc?
Yes, there should be `DiagStatePoints` from modules, but support for that seems
to basically be unimplemented at this point.
I believe the `DiagStatePoints` for macros are coming from a `_Pragma("clang
diagnostic ...")` within the macro expansion. Doesn't look like there's any
other way we'd create them.
https://reviews.llvm.org/D28207
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26893: [Sema] Fix assert on valid during template argument deduction
rsmith added a comment. Sorry I missed this =( Thanks for the fix! https://reviews.llvm.org/D26893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28258: [Sema] Handle invalid noexcept expressions correctly.
rsmith added a comment. Looks OK. Is it possible to add a test case for this without https://reviews.llvm.org/D20428? If not, this is small enough that rolling it into https://reviews.llvm.org/D20428 (so it can be committed with its testcase) would make sense. https://reviews.llvm.org/D28258 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28427: Allow constexpr construction of subobjects unconditionally, not just in C++14.
rsmith added a comment. Can you add a test please? https://reviews.llvm.org/D28427 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26244: [Driver] Prefer libraries installed next to Clang over those from GCC
rsmith added a comment. This makes sense to me in principle, but I'd like @chandlerc to weigh in. https://reviews.llvm.org/D26244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28473: Implement http://wg21.link/P0426 Constexpr for std::char_traits
rsmith added inline comments. Comment at: include/__string:248 +#if _LIBCPP_STD_VER <= 14 +return (const char_type*) memchr(__s, to_int_type(__a), __n); +#else We can add another builtin to Clang to support this case if you'd like. (There's also a way to get `__builtin_memchr` to work here using a `__builtin_constant_p` conditional expression to enable constant folding -- http://melpon.org/wandbox/permlink/0ob1n4a3zv1Kt3Ds -- but from discussion on IRC it sounds like Marshall is not happy about that approach.) https://reviews.llvm.org/D28473 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28427: Allow constexpr construction of subobjects unconditionally, not just in C++14.
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D28427 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC
rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/Lex/HeaderSearch.cpp:1082
+ auto TryEnterImported = [&](void) -> bool {
+if (!ModulesEnabled)
Maybe add a FIXME here indicating that this is a workaround for the lack of
proper modules-aware support for `#import` / `#pragma once`?
https://reviews.llvm.org/D26267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28399: PR31469: Don't add friend template class decls to redecl chain in dependent contexts.
rsmith added inline comments. Comment at: lib/Sema/SemaTemplate.cpp:1214-1215 CXXRecordDecl::Create(Context, Kind, SemanticContext, KWLoc, NameLoc, Name, PrevClassTemplate? PrevClassTemplate->getTemplatedDecl() : nullptr, /*DelayTypeCreation=*/true); You should keep the class out of its redeclaration chain too. Repository: rL LLVM https://reviews.llvm.org/D28399 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28505: CGDecl: Skip static variable initializers in unreachable code
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Yes, this is correct; per [stmt.dcl]/5, the destructor only runs if the object was constructed. Repository: rL LLVM https://reviews.llvm.org/D28505 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28510: Reinstate CWG1607 restrictions on lambdas appearing inside certain constant-expressions
rsmith added inline comments. Comment at: include/clang/Parse/Parser.h:1432 + ExprResult ParseConstantExpression(TypeCastState isTypeCast = NotTypeCast, + bool IsLambdaExprForbidden = false); ExprResult ParseConstraintExpression(); I would call this `IsInSignature`. Comment at: include/clang/Sema/Sema.h:894-895 +/// \brief Whether lambda expressions are forbidden here. +bool IsLambdaExprForbidden; + Rather than adding a flag, how about we have two different kinds of `ExpressionEvaluationContext` for constant expressions: a generic "constant expression" context and a "constant expression in signature" context? Repository: rL LLVM https://reviews.llvm.org/D28510 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rsmith added a comment.
Thanks! I assume there's still no measurable performance impact?
Comment at: include/clang/AST/ODRHash.h:54
+ // in Pointers.
+ size_type NextFreeIndex;
+
Is this always the same as `Pointers.size()`?
Comment at: lib/AST/ODRHash.cpp:35
+ for (auto base : Record->bases()) {
+ID.AddInteger(base.isVirtual());
+AddQualType(base.getType());
AddBoolean for clarity maybe?
Comment at: lib/AST/ODRHash.cpp:335-337
+ llvm::SmallVector Decls(DC->decls_begin(),
+DC->decls_end());
+ ID.AddInteger(Decls.size());
You will presumably need to filter out the implicit declarations before you
emit the size of the list, otherwise a `DeclContext` with an implicit
declaration will hash differently from one without.
Comment at: lib/AST/ODRHash.cpp:493-495
+ID.AddBoolean(hasDefaultArgument);
+if (hasDefaultArgument)
+ AddStmt(D->getDefaultArgument());
Given that `AddStmt` already writes out an "is null" flag, could you use
`AddStmt(hasDefaultArgument ? D->getDefaultArgument() : nullptr)` here?
Comment at: lib/Serialization/ASTReader.cpp:9015-9027
+if (FirstEnum->getName() != SecondEnum->getName()) {
+ Diag(FirstEnum->getLocStart(),
+ diag::err_module_odr_violation_mismatch_decl_diff)
+ << Merge.first << FirstModule.empty()
+ << FirstEnum->getSourceRange() << FirstModule << EnumName
+ << FirstEnum;
+ Diag(SecondEnum->getLocStart(),
Can you factor this out into a local lambda or helper class? These dozen lines
are repeated quite a lot of times here with only small variations.
https://reviews.llvm.org/D21675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27486: Correct class-template deprecation behavior
rsmith added inline comments. Comment at: include/clang/Basic/Attr.td:301 bit DuplicatesAllowedWhileMerging = 0; + // Set to true if this attribute should apply to template declarations, + // remains false if this should only be applied to the definition. I find this confusing -- it seems to suggest the attribute would be applied to the template declaration, not the templated declaration. I also think that the property we're modelling here is something more general than something about templates -- rather, I think the property is "is this attribute only meaningful when applied to / inherited into a defintiion?" It would also be useful to make clear that this only applies to class templates; for function templates, we always instantiate all the attributes with the declaration. Looking through our current attribute set, it looks like at least `AbiTag` should also get this set, and maybe also `Visibility`. (Though I wonder if there would be any problem with always instantiating the full attribute set for a class declaration.) Comment at: lib/Sema/SemaTemplate.cpp:2406-2407 + TemplateArgLists.addOuterTemplateArguments(Converted); + InstantiateAttrsForDecl(TemplateArgLists, + ClassTemplate->getTemplatedDecl(), Decl); ClassTemplate->AddSpecialization(Decl, InsertPos); You should also presumably do this when instantiating a member `CXXRecordDecl` nested within a class template, and when instantiating a local class in a function template. What should happen if more attributes are added between uses of the template? Example: ``` template struct A; A *p; template struct [[deprecated]] A; A *q; // warn here? ``` https://reviews.llvm.org/D27486 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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` that limits it to `F.Kind ==
> MK_ImplicitModule`, but I'm not sure it's necessary. Maybe it hits when
> reading PCH files, but no tests fail, and I'm not sure this is worse...
> thoughts?
For the PCH and preamble cases, `PCHValidator` should check that the diagnostic
mappings are the same prior to applying the diagnostic pragmas, so there should
never be a case where a warning mapping is upgraded to error/fatal error and
wasn't when the PCH was built (or vice versa) except when building with
`-fno-validate-pch`. Even in that case, the emergent behavior here seems mostly
OK (you imagine what the PCH would have looked like if it were built with the
current compilation's warning flags), except...
If you build a PCH that contains `#pragma clang diagnostic warning "-Wfoo"` and
then use it from a `-Werror=foo` compilation, it looks like we won't notice
that we need to upgrade the warning to an error when replaying the PCH's pragma
mappings. This is a corner case of a corner case, though.
> - If ReadDiagState sees a back-reference, it doesn't bother to check whether
> pragmas should be demoted; it assumes it should match whatever was done with
> the back-reference. I think this could be exercised with -Werror=X on the
> command-line and pragmas modifying -WX (first "ignored", then "error", then
> "warning"). Perhaps I should add a FIXME or a comment, but otherwise I think
> this is okay to miss...
IIRC we only get backreferences from pragma push/pop (and `CurDiagState`), and
I think the push/pop cases will always have the same upgrade behavior (you
can't push inside a module and pop outside it, for instance, so the starting
state for the push and pop should be consistent).
> It could be a back-reference to CurDiagState, which current gets
> (de)serialized before the pragma mappings. If we instead (de)serialize
> CurDiagState last, I think this one goes away. Is there a problem with that?
I don't think so, and putting `CurDiagState` last seems better in general (it
keeps the states in something much more like source order). I think I only put
it second for convenience (so I didn't need to check whether I'd just read the
last state in order to handle it differently).
Comment at: clang/include/clang/Basic/DiagnosticIDs.h:119-120
+
+ bool wasUpgradedFromWarning() const { return WasUpgradedFromWarning; }
+ void setUpgradedFromWarning(bool Value) { WasUpgradedFromWarning = Value; }
+
This could do with a documentation comment. Something like "Whether this
mapping attempted to map the diagnostic to a warning but was overruled because
the diagnostic was already mapped to an error or fatal error."
https://reviews.llvm.org/D30954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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&id=92588#toc
Repository:
rL LLVM
https://reviews.llvm.org/D31069
Files:
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/test/SemaCXX/P30636.cpp
Index: cfe/trunk/test/SemaCXX/P30636.cpp
===
--- cfe/trunk/test/SemaCXX/P30636.cpp
+++ cfe/trunk/test/SemaCXX/P30636.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wimplicit-fallthrough %s
+// expected-no-diagnostics
+
+template
+int fallthrough_template(int i)
+{
+ switch (i) {
+case 1:
+ if (param)
+return 3;
+ [[clang::fallthrough]]; // no warning here, for an unreachable
annotation (in the fallthrough_template case) in a template function
+case 2:
+ return 4;
+default:
+ return 5;
+ }
+}
+
+template int fallthrough_template(int);
+
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -972,7 +972,8 @@
}
}
-bool checkFallThroughIntoBlock(const CFGBlock &B, int &AnnotatedCnt) {
+bool checkFallThroughIntoBlock(const CFGBlock &B, int &AnnotatedCnt,
+ bool IsTemplateInstantiation) {
assert(!ReachableBlocks.empty() && "ReachableBlocks empty");
int UnannotatedCnt = 0;
@@ -1002,8 +1003,12 @@
ElemIt != ElemEnd; ++ElemIt) {
if (Optional CS = ElemIt->getAs()) {
if (const AttributedStmt *AS = asFallThroughAttr(CS->getStmt()))
{
-S.Diag(AS->getLocStart(),
- diag::warn_fallthrough_attr_unreachable);
+// Don't issue a warning for an unreachable fallthrough
+// attribute in template instantiations as it may not be
+// unreachable in all instantiations of the template.
+if (!IsTemplateInstantiation)
+ S.Diag(AS->getLocStart(),
+ diag::warn_fallthrough_attr_unreachable);
markFallthroughVisited(AS);
++AnnotatedCnt;
break;
@@ -1164,7 +1169,11 @@
int AnnotatedCnt;
-if (!FM.checkFallThroughIntoBlock(*B, AnnotatedCnt))
+bool IsTemplateInstantiation = false;
+if (const FunctionDecl *Function = dyn_cast(AC.getDecl()))
+ IsTemplateInstantiation = Function->isTemplateInstantiation();
+if (!FM.checkFallThroughIntoBlock(*B, AnnotatedCnt,
+ IsTemplateInstantiation))
continue;
S.Diag(Label->getLocStart(),
Index: cfe/trunk/test/SemaCXX/P30636.cpp
===
--- cfe/trunk/test/SemaCXX/P30636.cpp
+++ cfe/trunk/test/SemaCXX/P30636.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wimplicit-fallthrough %s
+// expected-no-diagnostics
+
+template
+int fallthrough_template(int i)
+{
+ switch (i) {
+case 1:
+ if (param)
+return 3;
+ [[clang::fallthrough]]; // no warning here, for an unreachable annotation (in the fallthrough_template case) in a template function
+case 2:
+ return 4;
+default:
+ return 5;
+ }
+}
+
+template int fallthrough_template(int);
+
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -972,7 +972,8 @@
}
}
-bool checkFallThroughIntoBlock(const CFGBlock &B, int &AnnotatedCnt) {
+bool checkFallThroughIntoBlock(const CFGBlock &B, int &AnnotatedCnt,
+ bool IsTemplateInstantiation) {
assert(!ReachableBlocks.empty() && "ReachableBlocks empty");
int UnannotatedCnt = 0;
@@ -1002,8 +1003,12 @@
ElemIt != ElemEnd; ++ElemIt) {
if (Optional CS = ElemIt->getAs()) {
if (const AttributedStmt *AS = asFallThroughAttr(CS->getStmt())) {
-S.Diag(AS->getLocStart(),
- diag::warn_fallthrough_attr_unreachable);
+// Don't issue a warning for an unreachable fallthrough
+// attribute in template instantiations as it may not be
+// unreachable in all instantiations of the template.
+if (!IsTemplateInstantiation)
+ S.Diag(AS->getLocStart(),
+ diag::warn_fallthrough_attr_unreachable);
markFallthroughVisited(AS);
[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 case, adding a full-blown UnitTest check for this seems like overkill... https://reviews.llvm.org/D31187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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())
neither -> none, specs -> specializations ("specs" makes me think
"specifications").
Comment at: lib/Sema/Sema.cpp:503
+if (VarTemplateDecl *Template = VD->getDescribedVarTemplate())
+ // If this is a variable template and neither of its specs is used, warn.
+ for (const auto *Spec : Template->specializations())
As above.
Comment at: lib/Sema/SemaDecl.cpp:1496
return false;
+ // 'static operator' functions are defined in headers; don't warn.
+ if (FD->isOverloadedOperator() &&
v.g.vassilev wrote:
> rsmith wrote:
> > Why? Defining a static operator in a header sounds like a bug to me.
> It seems we have some of these here:
>
> include/llvm/ADT/PointerUnion.h:static bool operator==(PointerUnion
> lhs, PointerUnion rhs) {
> include/llvm/ADT/PointerUnion.h:static bool operator!=(PointerUnion
> lhs, PointerUnion rhs) {
> include/llvm/ADT/PointerUnion.h:static bool operator<(PointerUnion
> lhs, PointerUnion rhs) {
> include/llvm/Transforms/Utils/ValueMapper.h:static inline RemapFlags
> operator|(RemapFlags LHS, RemapFlags RHS) {
>
> If that's a bug, I will remove this check.
Yes, those are bugs.
https://reviews.llvm.org/D29877
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 variable|option}0 '%1' is set">;
def err_drv_invalid_mfloat_abi : Error<
mehdi_amini wrote:
> Is it worth it? What about `"failing because %1 is set">;`
>
> And then later:
>
> ```
> Diags.Report(diag::err_drv_force_crash) << "option '-gen-reproducer'";
> ```
Even though we don't have translations for our diagnostics, it's intended that
they be translatable, so you should not hardcode strings like "option " and
"environment variable " in the code. Use a %select here instead, maybe? (See
http://clang.llvm.org/docs/InternalsManual.html#the-format-string)
https://reviews.llvm.org/D27604
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 deserialize the SLocEntry for every FileID referenced by a RawComment? That would seem pretty bad. https://reviews.llvm.org/D27546 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/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&id=95222#toc
Repository:
rL LLVM
https://reviews.llvm.org/D27263
Files:
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/Sema/expr-address-of.c
cfe/trunk/test/SemaCXX/ptrtomember.cpp
Index: cfe/trunk/test/SemaCXX/ptrtomember.cpp
===
--- cfe/trunk/test/SemaCXX/ptrtomember.cpp
+++ cfe/trunk/test/SemaCXX/ptrtomember.cpp
@@ -13,9 +13,13 @@
struct S2 {
int bitfield : 1;
+ struct {
+int anon_bitfield : 1;
+ };
};
int S2::*pf = &S2::bitfield; // expected-error {{address of bit-field
requested}}
+int S2::*anon_pf = &S2::anon_bitfield; // expected-error {{address of
bit-field requested}}
struct S3 {
void m();
Index: cfe/trunk/test/Sema/expr-address-of.c
===
--- cfe/trunk/test/Sema/expr-address-of.c
+++ cfe/trunk/test/Sema/expr-address-of.c
@@ -102,8 +102,9 @@
register struct {char* x;} t1 = {"Hello"};
char* dummy1 = &(t1.x[0]);
- struct {int a : 10;} t2;
+ struct {int a : 10; struct{int b : 10;};} t2;
int* dummy2 = &(t2.a); // expected-error {{address of bit-field requested}}
+ int* dummy3 = &(t2.b); // expected-error {{address of bit-field requested}}
void* t3 = &(*(void*)0);
}
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -1772,7 +1772,10 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, E->getLocStart()))
recordUseOfEvaluatedWeak(E);
- if (FieldDecl *FD = dyn_cast(D)) {
+ FieldDecl *FD = dyn_cast(D);
+ if (IndirectFieldDecl *IFD = dyn_cast(D))
+FD = IFD->getAnonField();
+ if (FD) {
UnusedPrivateFields.remove(FD);
// Just in case we're building an illegal pointer-to-member.
if (FD->isBitField())
Index: cfe/trunk/test/SemaCXX/ptrtomember.cpp
===
--- cfe/trunk/test/SemaCXX/ptrtomember.cpp
+++ cfe/trunk/test/SemaCXX/ptrtomember.cpp
@@ -13,9 +13,13 @@
struct S2 {
int bitfield : 1;
+ struct {
+int anon_bitfield : 1;
+ };
};
int S2::*pf = &S2::bitfield; // expected-error {{address of bit-field requested}}
+int S2::*anon_pf = &S2::anon_bitfield; // expected-error {{address of bit-field requested}}
struct S3 {
void m();
Index: cfe/trunk/test/Sema/expr-address-of.c
===
--- cfe/trunk/test/Sema/expr-address-of.c
+++ cfe/trunk/test/Sema/expr-address-of.c
@@ -102,8 +102,9 @@
register struct {char* x;} t1 = {"Hello"};
char* dummy1 = &(t1.x[0]);
- struct {int a : 10;} t2;
+ struct {int a : 10; struct{int b : 10;};} t2;
int* dummy2 = &(t2.a); // expected-error {{address of bit-field requested}}
+ int* dummy3 = &(t2.b); // expected-error {{address of bit-field requested}}
void* t3 = &(*(void*)0);
}
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -1772,7 +1772,10 @@
!Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, E->getLocStart()))
recordUseOfEvaluatedWeak(E);
- if (FieldDecl *FD = dyn_cast(D)) {
+ FieldDecl *FD = dyn_cast(D);
+ if (IndirectFieldDecl *IFD = dyn_cast(D))
+FD = IFD->getAnonField();
+ if (FD) {
UnusedPrivateFields.remove(FD);
// Just in case we're building an illegal pointer-to-member.
if (FD->isBitField())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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]>;
aaron.ballman wrote:
> I cannot see any documentation on MSDN for this spelling of the attribute,
> and MSVC 2015 rejects it. What is the motivating use case for this spelling?
Also, it seems rather unlikely that MSVC would implement the GNU inline
semantics. Have you investigated the actual semantic effects of this
`__declspec` attribute and checked they match the GNU semantics?
https://reviews.llvm.org/D32092
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 -O2, inline happens. What do you mean, "inline and declspec(inline) have the same behavior"? What exactly did you test? The `inline` keyword means quite different things in different language modes, and none of them are the same as the meaning of `__attribute__((gnu_inline))` (which is what you aliased this to). Some of these differences are quite subtle (for instance, the precise behavior of `extern inline` and `static inline`, and what happens if you redeclare a function with a different set of specifiers). https://reviews.llvm.org/D32092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 definitely wrong, and this should probably be setting the `inline` flag on the function. But I agree with Aaron: we need some evidence that implementing this in Clang is worthwhile (rather than changing the codebase to use the `inline` keyword instead). We aim to be CL-compatible for common code patterns, not to be 100% compatible with all code that CL accepts. https://reviews.llvm.org/D32092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26057: [coroutines] Add DependentCoawaitExpr and fix re-building CoroutineBodyStmt.
rsmith added inline comments.
Comment at: include/clang/AST/ExprCXX.h:4237
+ /// compiler.
+ bool IsImplicitlyCreated : 1;
+
I would go with just `isImplicit`, to match other similar uses throughout the
AST. Also maybe sink this into the `Stmt` bitfields to make this class 8 bytes
smaller
Comment at: lib/AST/ItaniumMangle.cpp:3302-3303
case Expr::AddrLabelExprClass:
+ // This should no longer exist in the AST by now
+ case Expr::DependentCoawaitExprClass:
case Expr::DesignatedInitUpdateExprClass:
I don't think "should no longer exist" is true. If `co_await` can appear in a
function signature at all, it can appear with a dependent operand. This should
be mangled the same as a non-dependent `co_await` expression.
Comment at: lib/Sema/SemaCoroutine.cpp:33
/// function type.
static QualType lookupPromiseType(Sema &S, const FunctionProtoType *FnType,
+ SourceLocation KwLoc,
EricWF wrote:
> The changes to this function are all unrelated cleanup/improvements.
Just go ahead and commit these separately then.
Comment at: lib/Sema/SemaCoroutine.cpp:99
+
+ auto buildNNS = [&]() {
auto *NNS = NestedNameSpecifier::Create(S.Context, nullptr, StdExp);
`buildElaboratedType` would be a better name for what this does. I also wonder
if this is really necessary, or whether we can just use %q0 in the diagnostic
format to request a fully-qualified type name.
Comment at: lib/Sema/SemaCoroutine.cpp:112-116
+ if (S.RequireCompleteType(FuncLoc, buildNNS(),
+diag::err_coroutine_promise_type_incomplete))
+return QualType();
return PromiseType;
We used to return the `ElaboratedType` and don't do so any more.
Comment at: lib/Sema/SemaCoroutine.cpp:409-414
+ return BuildDependentCoawaitExpr(Loc, E,
+ cast(Lookup.get()));
+}
+
+ExprResult Sema::BuildDependentCoawaitExpr(SourceLocation Loc, Expr *E,
+ UnresolvedLookupExpr *Lookup) {
This seems like an odd naming choice. I'd expect `BuildDependentCoawaitExpr` to
only deal with the case where the expression is dependent (and to never be
called otherwise), and `BuildCoawaitExpr` to handle both the case where the
expression is dependent and the case where it is non-dependent. Maybe the other
function should be called `BuildResolvedCoawaitExpr` or similar.
Comment at: lib/Sema/SemaDecl.cpp:11386
- // If we don't have a visible definition of the function, and it's inline or
+ // If we don't have a viNsible definition of the function, and it's inline or
// a template, skip the new definition.
Stray change.
Comment at: lib/Sema/TreeTransform.h:6687-6689
+ // Set that we have (possibly-invalid) suspend points before we do anything
+ // that may fail.
+ ScopeInfo->setCoroutineSuspendsInvalid();
Please use a term other than "invalid" here. We generally use "invalid" to mean
"an error has been diagnosed, use best-effort recovery".
Comment at: lib/Sema/TreeTransform.h:6802
+ return getDerived().RebuildDependentCoawaitExpr(
+ E->getKeywordLoc(), Result.get(), E->getOperatorCoawaitLookup());
+}
You need to transform the UnresolvedLookupExpr here too. (Example: we might
have a function-local declaration of `operator co_await` that needs to be
transformed into the version in the instantiated function.)
https://reviews.llvm.org/D26057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, looks good assuming your performance testing doesn't uncover anything. Comment at: lib/AST/ODRHash.cpp:319-321 +if (!D) return; +if (D->isImplicit()) + return; I think you can remove these lines: no-one should be calling this function with a null declaration or an implicit declaration. https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28931: Disable aligned new/delete on Apple platforms without posix_memalign
rsmith added a comment. Does OS X have the C11 `aligned_alloc` function? Perhaps we could use that instead, when available. https://reviews.llvm.org/D28931 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29027: [Stack Protection] Add remark for reasons why Stack Protection has been applied
rsmith added inline comments.
Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:229
+def remark_ssp_applied_reason
+: Remark<"SSP applied to function due to %select{an unknown reason|a "
+ "call to alloca|a stack allocated buffer or struct containing a "
Can this "unknown reason" case actually happen? It seems like a bug that SSP
insertion would not know why it's doing what it's doing.
Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:229
+def remark_ssp_applied_reason
+: Remark<"SSP applied to function due to %select{an unknown reason|a "
+ "call to alloca|a stack allocated buffer or struct containing a "
rsmith wrote:
> Can this "unknown reason" case actually happen? It seems like a bug that SSP
> insertion would not know why it's doing what it's doing.
Rather than the potentially-opaque initialism SSP, could you say "stack
protector" here? That would match the flag name better.
Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:230
+: Remark<"SSP applied to function due to %select{an unknown reason|a "
+ "call to alloca|a stack allocated buffer or struct containing a "
+ "buffer|the address of a local variable being taken|a function "
Do we actually know that the cause is a call to `alloca` rather than a VLA?
Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:232
+ "buffer|the address of a local variable being taken|a function "
+ "attribute or use of -fstack-protector-all}0">,
+ InGroup;
These two cases seem very easy to tell apart: if `-fstack-protector-all` is
specified, use that diagnostic, otherwise the LLVM attribute must have been
from a source-level attribute.
Comment at: include/clang/Basic/DiagnosticGroups.td:911
+// function.
+def SSPReason : DiagGroup<"ssp-reason">;
The flags to control this are `-fstack-protector*`, so `-Rstack-protector` (or
something starting `-Rstack-protector`) should be used here.
https://reviews.llvm.org/D29027
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28889: Change where we handle arg-dependent diagnose_if attributes
rsmith added inline comments. Comment at: include/clang/Sema/Overload.h:758 /// instead. +/// FIXME: Now that it only alloates ImplicitConversionSequences, do we want +/// to un-generalize this? Typo "alloates" Comment at: lib/Sema/SemaChecking.cpp:2490 CallType); + diagnoseArgDependentDiagnoseIfAttrs(FDecl, /*ThisArg=*/nullptr, Args, Loc); } Can this be moved inside `checkCall`? https://reviews.llvm.org/D28889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.
rsmith added a comment.
Generally looks good, but we have a better way of modeling types with a
trailing variable-length array that you should use.
Comment at: include/clang/AST/StmtCXX.h:299
/// down the coroutine frame.
class CoroutineBodyStmt : public Stmt {
enum SubStmt {
Please use `llvm::TrailingObjects` to store the trailing variable-length
`SubStmts` array.
Comment at: lib/Sema/SemaCoroutine.cpp:714-722
+// Try to form 'p.set_exception(std::current_exception());' to handle
+// uncaught exceptions.
+// TODO: Post WG21 Issaquah 2016 renamed set_exception to unhandled_exception
+// TODO: and dropped exception_ptr parameter. Make it so.
+
+ if (!PromiseRecordDecl)
+return true;
Reindent comments.
https://reviews.llvm.org/D28835
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28510: Reinstate CWG1607 restrictions on lambdas appearing inside certain constant-expressions
rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.
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 will be part of a
`typedef` declaration or function declaration until you've finished parsing it
(when you're parsing the decl-specifiers in a declaration you don't know
whether you're declaring a function or a variable, and the `typedef` keyword
might appear later).
So I think you need a different approach here. How about tracking the set of
contained lambdas on the `Declarator` and `DeclSpec` objects, and diagnose from
`ActOnFunctionDeclarator` / `ActOnTypedefDeclarator` if the current expression
evaluation context contains any lambdas? (Maybe when entering an expression
evaluation context, pass an optional `SmallVectorImpl*` to `Sema` to
collect the lambdas contained within the expression.)
There are some particularly "fun" cases to watch out for here:
decltype([]{})
a, // ok
f(); // ill-formed
... that require us to track the lambdas in the `DeclSpec` separately from the
lambdas in the `Declarator`.
Repository:
rL LLVM
https://reviews.llvm.org/D28510
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28772: [Preprocessor] Fix incorrect token caching that occurs when lexing _Pragma in macro argument pre-expansion mode when skipping a function body
rsmith added a comment. Can we instead address this locally in `_Pragma` handling, by getting it to clear out the junk it inserted into the token stream when it's done (if backtracking is enabled)? Repository: rL LLVM https://reviews.llvm.org/D28772 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28832: Improve redefinition errors pointing to the same header.
rsmith added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8708
+def note_redefinition_modules_same_file : Note<
+ "'%0' included multiple times, additional (likely non-modular) include
site in module '%1'">;
+def note_redefinition_modules_same_file_modulemap : Note<
If we can't be sure whether or not the other include side was a modular
include, making a guess is probably not helpful. (In fact, I've seen this issue
show up a lot more where the header is a modular header in both modules.)
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8710
+def note_redefinition_modules_same_file_modulemap : Note<
+ "consider adding '%0' as part of '%1' definition in">;
}
This note ends in the middle of a sentence.
Comment at: lib/Sema/SemaDecl.cpp:3731
+void Sema::diagnoseRedefinition(SourceLocation Old, SourceLocation New) {
+ SourceManager &SrcMgr = getSourceManager();
Can you give this a name that suggests that it produces a note rather than a
full diagnostic? `notePreviousDefinition`, maybe.
Comment at: lib/Sema/SemaDecl.cpp:3747
+ // is confusing, try to get better diagnostics when modules is on.
+ auto OldModLoc = SrcMgr.getModuleImportLoc(Old);
+ if (!OldModLoc.first.isInvalid()) {
Rather than listing where the module was first imported (which could be
unrelated to the problem), it would make more sense to list the location at
which the previous declaration was made visible. You can get that by querying
the `VisibleModuleSet` for the owning module of the definition and every other
module in its merged definitions set (see `Sema::hasVisibleMergedDefinition`).
Comment at: lib/Sema/SemaDecl.cpp:3755-3759
+ if (!Mod->DefinitionLoc.isInvalid())
+Diag(Mod->DefinitionLoc,
+ diag::note_redefinition_modules_same_file_modulemap)
+<< SrcMgr.getFilename(SrcMgr.getSpellingLoc(Old)).str()
+<< Mod->getFullModuleName();
Is this ("add the header to the module map for some other module that happened
to include it first") really generally good advice? People have a tendency to
follow such guidance blindly.
Comment at: lib/Sema/SemaDecl.cpp:3763
+ }
+} else { // Redefinitions caused by the lack of header guards.
+ Diag(IncludeLoc, diag::note_redefinition_same_file)
I don't think this should be an `else`. If both locations were textually
included in the current translation, we should still produce the "missing
include guard" diagnostic. Conversely, it'd seem reasonable to ask the
preprocessor if the header has an include guard before claiming that it doesn't.
https://reviews.llvm.org/D28832
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22587: [ASTContext] Fix part of DR224 for nontype template arguments
rsmith added a comment. In https://reviews.llvm.org/D22587#551647, @mgehre wrote: > I'm sorry if this sounds dumb, but is there a way for me to follow that > particular discussion? Only if you have access to the C++ committee's internal reflectors. Sadly this is not on the issues list yet either (it only lists issues reported up to the end of June 2016, and this was filed in July). The suggestion from the end of the discussion thread was that the right rule for recognising an injected-class-name is that the template argument must be equivalent to the template parameter for the primary template / template argument for a partial specialization, using the rules in [temp.over.link] to determine equivalence. (Thus not even parentheses surrounding a non-type template argument are permitted.) There seems to be consensus that dr224 was a mistake. https://reviews.llvm.org/D22587 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21675: New ODR checker for modules
rsmith added a comment. In https://reviews.llvm.org/D21675#654659, @teemperor wrote: > Would be nice if we could make Stmt::Profile, ODRHash and the CloneDetection > use the same backend. This code is *already* reusing the Stmt::Profile code for hashing of statements. Why was a different mechanism invented for `CloneDetection`? If it doesn't have different requirements, maybe it should be rewritten in terms of the `Stmt::Profile` implementation too. https://reviews.llvm.org/D21675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26345: Extend small data threshold driver options to PPC target
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. As of r293254, the `-G=` and `-msmall-data-threshold=` flags are just aliases of `-G`, so you don't need those parts of this patch any more. The PPC part looks fine, but please add a testcase. In future, please add cfe-commits as a subscriber to Clang changes; otherwise mail doesn't get sent there and patches tend to get lost. https://reviews.llvm.org/D26345 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26345: Extend small data threshold driver options to PPC target
rsmith added a comment. LGTM https://reviews.llvm.org/D26345 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28889: Change where we handle arg-dependent diagnose_if attributes
rsmith added a comment.
Another "fun" testcase:
struct S {
void operator++(int n) _diagnose_if(n, "wat", "warning");
};
void f(S s) {
s++; // no warning
s.operator++(1); // warning
}
Comment at: include/clang/Sema/Sema.h:2638
- /// Check the diagnose_if attributes on the given function. Returns the
- /// first succesful fatal attribute, or null if calling Function(Args) isn't
- /// an error.
+ /// Emit diagnostics for the diagnose_if attributes on Function, ignoring any
+ /// non-ArgDependent DiagnoseIfAttrs.
Can you add a comment somewhere about here explaining why these functions are
split? Something like "Argument-dependent diagnose_if attributes are checked
when the function is used as a direct callee of a function call." here, and
"Argument-independent diagnose_if attributes are checked on every use of the
function." below.
Comment at: include/clang/Sema/Sema.h:9925
SourceLocation Loc, SourceRange Range,
- VariadicCallType CallType);
+ VariadicCallType CallType, const Expr *ThisArg);
We have a loose convention that function parameter argument order matches
source order, which would suggest that `ThisArg` should precede `Args` here.
Comment at: lib/Sema/SemaExprCXX.cpp:6717
+
+ checkDiagnoseIfAttrsOnCall(Method, CE);
return CE;
Can you call `CheckFunctionCall` instead here, and remove
`checkDiagnoseIfAttrsOnCall`? It looks like the only reason we don't already
call that is because none of its checks could ever fire for a call to a
conversion function before, and that's no longer true.
Comment at: lib/Sema/SemaOverload.cpp:12035
+ checkDiagnoseIfAttrsOnCall(FnDecl, TheCall);
return MaybeBindToTemporary(TheCall);
Likewise call `CheckFunctionCall` here.
Comment at: lib/Sema/SemaOverload.cpp:12488
+checkDiagnoseIfAttrsOnCall(Method, TheCall);
return MaybeBindToTemporary(TheCall);
... and here.
Comment at: lib/Sema/SemaOverload.cpp:13228
+ checkDiagnoseIfAttrsOnCall(Method, TheCall);
return MaybeBindToTemporary(TheCall);
... and here.
Comment at: test/SemaCXX/diagnose_if.cpp:614
+// FIXME: This should emit diagnostics. It seems that our constexpr
+// evaluator isn't able to evaluate `adl::Foo(1)` to a constexpr, though.
+// I'm assuming this is because we assign it to a temporary.
`constexpr` is an adjective; "to a constant" might make more sense.
Comment at: test/SemaCXX/diagnose_if.cpp:615
+// evaluator isn't able to evaluate `adl::Foo(1)` to a constexpr, though.
+// I'm assuming this is because we assign it to a temporary.
+for (void *p : adl::Foo(1)) {}
The range-based for is desugared to
```
auto &&__range = adl::Foo(1);
auto __begin = begin(__range);
auto __end = end(__range);
// ...
```
so the argument in the call to `begin` is not considered constant.
https://reviews.llvm.org/D28889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28845: Prototype of modules codegen
rsmith added a comment.
Comment at: include/clang/AST/ASTContext.h:2490
/// it is not used.
- bool DeclMustBeEmitted(const Decl *D);
+ bool DeclMustBeEmitted(const Decl *D, bool WritingModule = false);
I think the name of this flag might be out of sync with its meaning; it looks
like it means "must this declaration be emitted when performing modular
codegen?"
Comment at: include/clang/Basic/Module.h:208
+ unsigned WithCodegen : 2;
+
Does this need to be two bits wide? Seems to only store true/false.
Comment at: include/clang/Driver/CC1Options.td:435-438
+def fmodule_codegen :
+ Flag<["-"], "fmodule-codegen">,
+ HelpText<"Generate code for uses of this module that assumes an explicit "
+ "object file will be built for the module">;
Maybe `module` -> `modules`, to match the other `-fmodules-*` flags controlling
various options.
Comment at: lib/AST/ASTContext.cpp:9020-9023
+ if (auto *Ext = getExternalSource())
+if (Ext->hasExternalDefinitions(FD->getOwningModuleID()) ==
+ExternalASTSource::EK_Never)
+ return true;
I think this `return true` is unreachable and can be deleted: if we get here
with `Linkage == GVA_DiscardableODR`, then the call to `hasExternalDefinitions`
in `GetGVALinkageForFunction` must have returned `EK_ReplyHazy`. (In the case
this is checking for, `GetGVALinkageForFunction` would return `GVA_StrongODR`,
so the check below will return `true` anyway.)
Comment at: lib/AST/ASTContext.cpp:9029
// Implicit template instantiations can also be deferred in C++.
return !isDiscardableGVALinkage(GetGVALinkageForFunction(FD));
}
Pass `Linkage` in here instead of recomputing it
Comment at: lib/AST/ExternalASTSource.cpp:33
+ExternalASTSource::hasExternalDefinitions(unsigned ID) {
+ return EK_ReplyHazy;
+}
You should add support for this function to
`clang::MultiplexExternalSemaSource` too.
Comment at: lib/Serialization/ASTWriterDecl.cpp:2215-2219
+ if (isRequiredDecl(D, Context, WritingModule, false))
EagerlyDeserializedDecls.push_back(ID);
+ else if (Context.getLangOpts().ModularCodegen && WritingModule &&
+ isRequiredDecl(D, Context, true, true))
+ModularCodegenDecls.push_back(ID);
I suspect we'll want to seriously prune back the contents of
`EagerlyDeserializedDecls` for the modular codegen case at some point, but we
don't need to do that in this change.
https://reviews.llvm.org/D28845
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28845: Prototype of modules codegen
rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/AST/ExternalASTSource.cpp:33
+ExternalASTSource::hasExternalDefinitions(unsigned ID) {
+ return EK_ReplyHazy;
+}
dblaikie wrote:
> rsmith wrote:
> > You should add support for this function to
> > `clang::MultiplexExternalSemaSource` too.
> Done - though is there any test coverage I should add? Not sure exactly
> when/where/how this is used.
>
> Also only made a rough guess at what the right behavior is here. It could be
> a bit more obvious if I made "hasExternalDefinitions" return
> Optional - then when we find an ExternalASTSource that owns the ID
> (are the IDs unique across the MultiplexExternalSemaSource? I assume they
> have to be, but thought they'd only be valid within a single Module... guess
> I'm confused in a few ways - certainly haven't thought about it in great
> detail) we'd know to stop asking other sources. Probably not worth it unless
> there's a lot of sources?
You could test this with `-chain-include`. I don't think there's any other
in-tree way to get multiple external sources.
https://reviews.llvm.org/D28845
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24333: [CleanupInfo] Use cleanupsHaveSideEffects instead of exprNeedsCleanups in assertions
rsmith added inline comments.
Comment at: clang/test/SemaCXX/pr30306.cpp:5
+template
+void g(T) { int a[f(3)]; } // expected-no-diagnostics
+
Shouldn't we be (somehow) handling the cleanups from the array bound expression
here -- either discarding them or attaching them to the array bound? Looks like
`TreeTransform::TransformVariableArrayType` is missing a call to
`ActOnFinishFullExpr`.
If we modify the test to:
```
struct A { A(int); ~A(); };
int f(const A &);
template void g() { int a[f(3)]; }
int main() { g(); }
```
... we need to register the `~A()` destructor to actually be run at some point.
https://reviews.llvm.org/D24333
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16135: Macro Debug Info support in Clang
rsmith added inline comments. Comment at: include/clang/CodeGen/ModuleBuilder.h:71 + /// + /// This methods can be called before initializing the CGDebugInfo calss. + /// But the returned value should not be used until after initialization. Typo 'calss' Comment at: include/clang/CodeGen/ModuleBuilder.h:72 + /// This methods can be called before initializing the CGDebugInfo calss. + /// But the returned value should not be used until after initialization. + /// It is caller responsibility to validate that the place holder was What is the returned value in that case? A reference to a null pointer? Generally, I'm not particularly happy with this approach of exposing a reference to an internal pointer as the way of propagating the `CGDebugInfo` to the macro generator as needed. Instead, how about giving the `MacroPPCallbacks` object a pointer to the `CodeGenerator` itself, and having it ask the `CodeGenModule` for the debug info object when it needs it? Comment at: include/clang/CodeGen/ModuleBuilder.h:73 + /// But the returned value should not be used until after initialization. + /// It is caller responsibility to validate that the place holder was + /// initialized before start using it. caller -> the caller's Comment at: include/clang/CodeGen/ModuleBuilder.h:74 + /// It is caller responsibility to validate that the place holder was + /// initialized before start using it. + CodeGen::CGDebugInfo *&CodeGenerator::getModuleDebugInfoRef(); start using -> starting to use Comment at: lib/CodeGen/MacroPPCallbacks.cpp:1 +//===--- MacroPPCallbacks.h -*- C++ -*-===// +// Wrong file header (should be .cpp, and you don't need the "-*- C++ -*-" in a .cpp file. https://reviews.llvm.org/D16135 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15994: Allow for unfinished #if blocks in preambles.
rsmith added inline comments.
Comment at: include/clang/Lex/Preprocessor.h:310
+
+const Stack &getStack() const {
+ assert(ConditionalStack);
Return an `ArrayRef` rather than exposing the type of the storage (which is an
implementation detail), here and transitively through the callers of this.
Comment at: include/clang/Lex/Preprocessor.h:322
+
+void setStack(const Stack &s) {
+ if (!isRecording() && !isReplaying())
Please pass in an `ArrayRef` here rather than a `SmallVector`.
Comment at: include/clang/Lex/Preprocessor.h:328
+ else
+ConditionalStack = new Stack(s);
+}
I don't see a need to heap-allocate this separately from the `Preprocessor`.
Comment at: include/clang/Lex/PreprocessorOptions.h:84
+
+ bool PreambleGeneration = false;
Please add a doc comment for this option. Also, maybe `GeneratePreamble` to
avoid ambiguity as to whether this is some kind of generation number for the
preamble?
Comment at: lib/Lex/PPLexerChange.cpp:139-142
+ if (PreambleConditionalStack.isReplaying()) {
+CurPPLexer->setConditionalLevels(PreambleConditionalStack.getStack());
+PreambleConditionalStack.doneReplaying();
+ }
I think this would make more sense two callers up, in
`Preprocessor::EnterMainSourceFile`
https://reviews.llvm.org/D15994
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21075: Correct invalid end location in diagnostics for some identifiers.
rsmith added inline comments. Comment at: lib/Parse/ParseDecl.cpp:2702 DS.SetRangeStart(Tok.getLocation()); -DS.SetRangeEnd(SourceLocation()); +DS.SetRangeEnd(Tok.getLocation()); } This doesn't look right: this is a source range containing exactly one token. If we don't have *any* decl-specifiers (such as for a constructor, destructor, or conversion function), this is the wrong range. How about instead setting the start location to be invalid in `DeclSpec::Finish` if the end location is invalid? Comment at: lib/Sema/SemaExpr.cpp:2061-2062 + auto Builder = Diag(R.getNameLoc(), diagnostic) << Name; + if (Name.isIdentifier()) +Builder << SourceRange(R.getNameLoc()); return true; erikjv wrote: > rsmith wrote: > > I'm indifferent on this change: I don't see much point in adding a > > single-token range when we already point the caret at that token, but I > > don't see any harm either. > It's mostly about how much is "underlined". If there is only a caret, that > quite often translates into a single character being pointed out, instead of > an identifier (i.e. the first character). Hene the extension of the range. A `^` with no `~`s should be interpreted as indicating a location, not highlighting a single character. We use that pattern in a huge number of diagnostics; it doesn't make sense to do something different here. I also don't think this is the correct range to underline. If the name comprises multiple tokens (such as `operator int`), this will incorrectly highlight only the first one. A wrong underline seems worse than no underline. https://reviews.llvm.org/D21075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25674: [Concepts] Class template associated constraints
rsmith added inline comments.
Comment at: include/clang/AST/DeclTemplate.h:373-391
+class TemplateDeclWithACBase {
+protected:
+ TemplateDeclWithACBase() = default;
+
+ ConstrainedTemplateDeclInfo CTDInfo;
+};
+
This mechanism seems unnecessary to me; allocating the
`ConstrainedTemplateDeclInfo` separately seems a lot simpler. Forcing this and
the template into a single allocation is unlikely to help anything since we use
a slab allocator (which is going to lay the objects out the same way this
template trick does, unless we hit the end of a slab).
Comment at: lib/Sema/SemaTemplate.cpp:1178
+ if (!(CurAC || PrevAC))
+return false; // nothing to check
+ if (CurAC && PrevAC) {
[nit] Comments should be full sentences: capitalized and ending in a period.
Comment at: lib/Sema/SemaTemplate.cpp:1299-1300
+ // Attach the associated constraints when the declaration will not be part of
+ // a decl chain
+ Expr *const ACtoAttach =
Is there a reason you don't want to store the associated constraints that were
specified on a redeclaration? I'd expect that to hurt tools that want source
fidelity (for instance, a renaming tool will want to be able to find all the
references to a particular name, even in a //requires-clause// on a
redeclaration of a template).
https://reviews.llvm.org/D25674
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29748: [cxx1z-constexpr-lambda] Implement captures - thus completing implementation of constexpr lambdas.
rsmith added inline comments.
Comment at: lib/AST/ExprConstant.cpp:5061
+ APValue RVal;
+ // FIXME: We need to make sure we're passing the right type that
+ // maintains cv-qualifiers.
faisalv wrote:
> I don't think we need this fixme - the type of the expression should be
> correct - all other const checks for mutability have already been performed,
> right?
When using `handleLValueToRValueConversion` to obtain the lvalue denoted by a
reference, the type you pass should be the reference type itself
(`FD->getType()`). This approach will give the wrong answer when using a
captured volatile object:
```
void f() {
volatile int n;
constexpr volatile int *p = [&]{ return &n; }(); // should be accepted
}
```
Comment at: lib/AST/ExprConstant.cpp:5066-5068
+ assert(RVal.isLValue() && "Reference captures through their "
+"corresponding field members must refer to
"
+"lvalues (VarDecls or FieldDecls)");
I don't see why this assert is correct. If we initialize a reference with a
(constant-folded) dereferenced integer cast to pointer type, the value will
have integer representation. Just remove the assert?
Comment at: lib/AST/ExprConstant.cpp:5473-5474
+
+ if (HandleLValueMember(Info, E, Result,
+ Info.CurrentCall->LambdaThisCaptureField)) {
+if (Info.CurrentCall->LambdaThisCaptureField->getType()
Please use early-exit style (`if (!HandleLValueMember(...)) return false;`)
here to reduce indentation and make it clearer that you only return false if a
diagnostic has already been produced.
Comment at: lib/AST/ExprConstant.cpp:6338-6339
+// occurred.
+if (!CurFieldInit)
+ return false;
+
Returning `false` without producing a diagnostic (for the VLA case) is not OK.
You should at least produce the basic "not a constant expression" note here.
Repository:
rL LLVM
https://reviews.llvm.org/D29748
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25674: [Concepts] Class template associated constraints
rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Sema/SemaTemplate.cpp:1300 + // Attach the associated constraints when the declaration will not be part of + // a decl chain + Expr *const ACtoAttach = Add period to this sentence. https://reviews.llvm.org/D25674 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22057: Prevent devirtualization of calls to un-instantiated functions.
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D22057#543199, @Sunil_Srivastava wrote: > Now: Why the InstantiationIsPending bit is not precisely tracking the > presence in the PendingInstantiations list? [...] > The instantiation loop removes items from the PendingInstantiations list and > instantiates them, if the body is present. In the case of Func2, the body is > not present, the function has already been removed from the list, yet it is > not isDefined(). I see, so the only case in which the list and the flag differs is when we're doing the final instantiation step at the end of the TU? (In all other cases, we'd put the function back on the list for later instantiations to retry.) That seems fine. https://reviews.llvm.org/D22057 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27486: Correct class-template deprecation behavior
rsmith added inline comments. Comment at: include/clang/Basic/Attr.td:301 bit DuplicatesAllowedWhileMerging = 0; + // Set to true if this attribute should apply to template declarations, + // remains false if this should only be applied to the definition. erichkeane wrote: > rsmith wrote: > > I find this confusing -- it seems to suggest the attribute would be applied > > to the template declaration, not the templated declaration. I also think > > that the property we're modelling here is something more general than > > something about templates -- rather, I think the property is "is this > > attribute only meaningful when applied to / inherited into a defintiion?" > > It would also be useful to make clear that this only applies to class > > templates; for function templates, we always instantiate all the attributes > > with the declaration. > > > > Looking through our current attribute set, it looks like at least `AbiTag` > > should also get this set, and maybe also `Visibility`. (Though I wonder if > > there would be any problem with always instantiating the full attribute set > > for a class declaration.) > > (Though I wonder if there would be any problem with always instantiating > > the full attribute set for a class declaration.) > > This is definitely a good point. It seems that just about every other usage > of instantiating attributes happens right after creation, class template > specialization is the lone exception it seems. > > If I were to simply revert most of this change, then alter my > SemaTemplate.cpp changes to just call InstantiateAttrs and presumably remove > the other call to InstantiateAttrs (since it results in 2 sets of > attributes), would you consider that to be more acceptable? > > > Yes, I think we should at least try that and see if there's a reason why we would need the extra complexity here. https://reviews.llvm.org/D27486 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29724: [Driver] Report available language standards on user error
rsmith added inline comments. Comment at: include/clang/Basic/DiagnosticDriverKinds.td:233 "AddressSanitizer doesn't support linking with debug runtime libraries yet">; +def note_drv_supported_values : Note<"supported values are:">; +def note_drv_supported_value_with_description : Note<"'%0' for standard '%1'">; Please remove this note and instead add "use " to the start of the next note. (That is more consistent with how we add notes to other diagnostics.) Comment at: lib/Frontend/CompilerInvocation.cpp:1709 +Diags.Report(diag::note_drv_supported_value_with_description) + << Std.getName() << Std.getDescription(); + } idlecode wrote: > ahatanak wrote: > > Is it possible to change the diagnostic so that it's easier to tell which > > part is the supported value and which part is the description? > > > > The diagnostic looks like this, and I found it a little hard to tell at a > > quick glance: > > > > "c89 - ISO C 1990" > Sure, I have tried few formats with quotes/colons but how about this (a bit > verbose) version: > ``` > note: supported values are: > note: 'c89' for standard 'ISO C 1990' > note: 'c90' for standard 'ISO C 1990' > note: 'iso9899:1990' for standard 'ISO C 1990' > ... > ``` > What do you think about it? Do you have any suggestions? We should probably suppress the notes for standards that aren't applicable to the current language. 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] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, I think this is also OK for Clang 4 if Hans is willing to take it. Comment at: lib/AST/ExprConstant.cpp:607-612 + /// Evaluate as a constant expression. In certain scenarios, if: + /// - We find a MemberExpr with a base that can't be evaluated, or /// - We find a variable initialized with a call to a function that has - /// the alloc_size attribute on it. + /// the alloc_size attribute on it + /// + /// Then we may consider evaluation to have succeeded. [nit] "We", "We", "Then" should not be capitalized, and please remove blank line prior to "Then we [...]" https://reviews.llvm.org/D29469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29863: [libc++] Fix PR 31938 - std::basic_string constructors use non-deductible parameter types.
rsmith added a comment. Looks good, though there are some `value_type` constructors left. I've not checked the standard to see if they are all declared with `charT`. Comment at: include/string:782 _LIBCPP_INLINE_VISIBILITY basic_string(const value_type* __s, size_type __n); _LIBCPP_INLINE_VISIBILITY Did you skip this one intentionally? Comment at: include/string:788 _LIBCPP_INLINE_VISIBILITY basic_string(size_type __n, value_type __c, const allocator_type& __a); basic_string(const basic_string& __str, size_type __pos, size_type __n, Likewise these two. Comment at: include/string:812 _LIBCPP_INLINE_VISIBILITY basic_string(initializer_list __il, const allocator_type& __a); #endif // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS And these https://reviews.llvm.org/D29863 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29724: [Driver] Report available language standards on user error
rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/Frontend/CompilerInvocation.cpp:1753-1754
+ KindValue != LangStandard::lang_unspecified;
+ ++KindValue)
+ {
+const LangStandard &Std = LangStandard::getLangStandardForKind(
`{` on previous line, please, and indent the previous two lines to match the
`(`.
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] 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
[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. `size_type` is a typedef for `allocator_traits::size_type`, > This causes the `basic_string(CharT*, Allocator const&)` to always be chosen > instead, resulting in a incorrect allocator type. I don't think it will always be chosen instead; if the argument is of type `size_t`, the `(const CharT*, size_type)` overload should be chosen. https://reviews.llvm.org/D29863 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 that adding this to every `CallStackFrame` may have a
nontrivial impact on the overall stack usage of deeply-recursing constexpr
evaluataions. (I'd also like to cache this map rather than recomputing it
repeatedly.) But let's try this and see how it goes; we can look into caching
the map as a later change.
Comment at: lib/AST/ExprConstant.cpp:4194
+MD->getParent()->getCaptureFields(Frame.LambdaCaptureFields,
+ Frame.LambdaThisCaptureField);
}
Indent.
Comment at: lib/AST/ExprConstant.cpp:5061
+ // ... then update it to refer to the field of the closure object
+ // that represents the capture.
+ if (!HandleLValueMember(Info, E, Result, FD))
```constexpr bool b = [&]{ return &n; }() == &n; // should be accepted```
... is more what I was thinking.
Comment at: lib/AST/ExprConstant.cpp:5067-5071
+ APValue RVal;
+ if (!handleLValueToRValueConversion(Info, E, FD->getType(), Result,
+ RVal))
+return false;
+ Result.setFrom(Info.Ctx, RVal);
Too much indentation here.
https://reviews.llvm.org/D29748
___
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 with a reference-specific side-check; it seems better to expose the underlying check itself and allow the user to figure out how they want to combine the checks. Perhaps we could introduce a `__reference_binds_to_temporary(T, U)` trait, where `T` is required to be a reference type, that determines whether a reference of type `T` bound to an expression of type `U` would bind to a materialized temporary object (or subobject thereof)? https://reviews.llvm.org/D29930 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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] 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 will be
> > part of a `typedef` declaration or function declaration until you've
> > finished parsing it (when you're parsing the decl-specifiers in a
> > declaration you don't know whether you're declaring a function or a
> > variable, and the `typedef` keyword might appear later).
>
>
>
>
>
> I see. The issue is that the current approach would forbid valid variable
> declarations such as:
>
> void (*f)(int [([]{return 5;}())]) = 0;
>
> ... where lambdas can appear within the array declarators (I believe that's
> the only syntactic neighborhood that can cause this problem, right?).
I think so, at least until Louis removes the restriction on lambdas in
unevaluated operands. Then we have a whole host of new problems:
decltype([]{}()) typedef x; // error, lambda in a typedef
template decltype([]{}()) f(); // error, lambda in a function
declaration
template decltype([]{}()) x; // ok
If we want a forward-looking change which prepares us for that, we should be
thinking about how to deal with deferring the check until we get to the end of
the declaration and find out whether we declared a function or a typedef.
> Well lambda's can't appear in unevaluated operands yet, so your example would
> be ill-formed? If so, we don't have to worry about them showing up in
> decl-specifiers?
> The only Declarator where they could be well formed is within an array
> declarator of a variable or a parameter of a function pointer variable (but
> no where else, i.e typedefs and function declarations), right?
Right. But we should at least keep in mind the upcoming removal of the
unevaluated operands restriction. Ideally, we would get some implementation
experience with that now, so we can make sure that we standardize a reasonable
set of rules.
https://reviews.llvm.org/D28510
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 how this patch actually addresses that problem, though: if you build this testcase as a module instead of a PCH, won't it still fail in the same way that it does now? I think we probably need to track all the declarations we deserialize in a top-level deserialization, and only check whether they're interesting when we finish recursive deserialization (somewhere around `ASTReader::FinishedDeserializing`). For the update record case, this will need some care in order to retain the property that we only pass a declaration to the consumer once, but I think we can make that work by observing that it should generally be safe to call `DeclMustBeEmitted` on a declaration that we didn't create in this deserialization step, and when we're loading update records we know whether that's the case. https://reviews.llvm.org/D29753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 implicit deduction.
+// const test_allocator alloc{};
I think that at least matches the standard as-is. I'm not sure this case is
worth adding an explicit deduction guide for. *shrug*
Comment at:
test/std/strings/basic.string/string.cons/implicit_deduction_guides.pass.cpp:107
+ { // Testing (5) w/o allocator
+#if 0 // FIXME: This doesn't work
+const std::string sin("abc");
Do you know why not?
Comment at:
test/std/strings/basic.string/string.cons/implicit_deduction_guides.pass.cpp:291
+ { // Testing (15)
+// FIXME: This overload is broken. Fix it and add tests.
+ }
I think the inability to deduce using this overload matches the standard. I
don't think there's any way in general to map the type `T` to a `charT`.
https://reviews.llvm.org/D29863
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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.
Reuse the Type::Auto case here rather than duplicating it. (You'll need to
change its AutoType to the DeducedType common base class.)
https://reviews.llvm.org/D30082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 Signature.
+ UNHASHED_CONTROL_BLOCK_ID,
This comment is out of date. Maybe just point to the
`UnhashedControlBlockRecordTypes` for the definitive list of records within
this block?
Comment at: clang/lib/Serialization/ASTReader.cpp:2240-2241
+ // Lambda to read the unhashed control block the first time it's called.
+ bool HasReadUnhashedControlBlock = false;
+ auto readUnhashedControlBlockOnce = [&]() {
I guess the reason to do this is because reading that block depends on certain
things in this block having been already read, and reading other things in this
block depends on that block having been read? A comment explaining that would
be useful.
Comment at: clang/lib/Serialization/ASTReader.cpp:4014-4015
+case UNHASHED_CONTROL_BLOCK_ID:
+ // This block is handled using look-ahead during ReadControlBlock. We
+ // shouldn't get here!
+ return Failure;
We shouldn't return `Failure` without producing an error message.
https://reviews.llvm.org/D27689
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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&id=89300#toc
Repository:
rL LLVM
https://reviews.llvm.org/D30082
Files:
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/test/CodeGenCXX/debug-info-template-deduction-guide.cpp
Index: cfe/trunk/test/CodeGenCXX/debug-info-template-deduction-guide.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-template-deduction-guide.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-template-deduction-guide.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang -S -emit-llvm -target x86_64-unknown_unknown -g %s -o -
-std=c++1z | FileCheck %s
+
+// Verify that we don't crash when emitting debug information for objects
+// created from a deduced template specialization.
+
+template
+struct S {
+ S(T) {}
+};
+
+// CHECK: !DIGlobalVariable(name: "s1"
+// CHECK-SAME: type: [[TYPE_NUM:![0-9]+]]
+// CHECK: !DIGlobalVariable(name: "s2"
+// CHECK-SAME: type: [[TYPE_NUM]]
+// CHECK: [[TYPE_NUM]] = distinct !DICompositeType(tag: DW_TAG_structure_type,
name: "S",
+S s1(42);
+S s2(42);
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -2475,8 +2475,9 @@
case Type::SubstTemplateTypeParm:
T = cast(T)->getReplacementType();
break;
-case Type::Auto: {
- QualType DT = cast(T)->getDeducedType();
+case Type::Auto:
+case Type::DeducedTemplateSpecialization: {
+ QualType DT = cast(T)->getDeducedType();
assert(!DT.isNull() && "Undeduced types shouldn't reach here.");
T = DT;
break;
Index: cfe/trunk/test/CodeGenCXX/debug-info-template-deduction-guide.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-template-deduction-guide.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-template-deduction-guide.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang -S -emit-llvm -target x86_64-unknown_unknown -g %s -o - -std=c++1z | FileCheck %s
+
+// Verify that we don't crash when emitting debug information for objects
+// created from a deduced template specialization.
+
+template
+struct S {
+ S(T) {}
+};
+
+// CHECK: !DIGlobalVariable(name: "s1"
+// CHECK-SAME: type: [[TYPE_NUM:![0-9]+]]
+// CHECK: !DIGlobalVariable(name: "s2"
+// CHECK-SAME: type: [[TYPE_NUM]]
+// CHECK: [[TYPE_NUM]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S",
+S s1(42);
+S s2(42);
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -2475,8 +2475,9 @@
case Type::SubstTemplateTypeParm:
T = cast(T)->getReplacementType();
break;
-case Type::Auto: {
- QualType DT = cast(T)->getDeducedType();
+case Type::Auto:
+case Type::DeducedTemplateSpecialization: {
+ QualType DT = cast(T)->getDeducedType();
assert(!DT.isNull() && "Undeduced types shouldn't reach here.");
T = DT;
break;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 struct E : X, T {
I don't see a reason to `#ifdef` this portion, which should work either way,
and likewise for the other change to this file. (The changes to the other
header and to the cpp file look fine and appropriate, though.)
Comment at: test/SemaCXX/warn-thread-safety-parsing.cpp:1273
+#if __cplusplus <= 199711L
+ // expected-error@-2 {{invalid use of member 'mu' in static member function}}
+#endif
Please add FIXMEs to this test. These cases are not supposed to be permitted.
https://reviews.llvm.org/D21626
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
