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

[Richard Smith via Phabricator via cfe-commits]

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] D29877: Warn about unused static file scope function template declarations.

[Richard Smith via Phabricator via cfe-commits]

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] D29877: Warn about unused static file scope function template declarations.

[Richard Smith via Phabricator via cfe-commits]

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] D33538: [coroutines] Support "coroutines" feature in module map requires clause

[Richard Smith via Phabricator via cfe-commits]

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] D33568: Fix crash when evaluating constant expressions involving nullptr

[Richard Smith via Phabricator via cfe-commits]

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

[Richard Smith via Phabricator via cfe-commits]

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] D33538: [coroutines] Support "coroutines" feature in module map requires clause

[Richard Smith via Phabricator via cfe-commits]

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

[Richard Smith via Phabricator via cfe-commits]

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] D33568: Fix crash when evaluating constant expressions involving nullptr

[Richard Smith via Phabricator via cfe-commits]

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] D31692: [coroutines] Wrap the body of the coroutine in try-catch

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D31692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


https://reviews.llvm.org/D9



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

This is supposed to be handled by `Sema::DeclareGlobalAllocationFunction`:

  DeclContext::lookup_result R = GlobalCtx->lookup(Name);
  for (DeclContext::lookup_iterator Alloc = R.begin(), AllocEnd = R.end();
   Alloc != AllocEnd; ++Alloc) {
// Only look at non-template functions, as it is the predefined,
// non-templated allocation function we are trying to declare here.
if (FunctionDecl *Func = dyn_cast(*Alloc)) {
  if (Func->getNumParams() == Params.size()) {
llvm::SmallVector FuncParams;
for (auto *P : Func->parameters())
  FuncParams.push_back(
  Context.getCanonicalType(P->getType().getUnqualifiedType()));
if (llvm::makeArrayRef(FuncParams) == Params) {
  // Make the function visible to name lookup, even if we found it in
  // an unimported module. It either is an implicitly-declared global
  // allocation function, or is suppressing that function.
  Func->setHidden(false);
  return;

... but it looks like we only get there once per compilation, which means that 
we won't unset the 'hidden' flag if we create the functions in one submodule 
and then need them from a second one, as in your testcase.

A better approach would be to call `Alloc->setHidden(false)` (and maybe also 
`Alloc->setLocalOwningModule(nullptr)`) after we create the implicit 
declaration in the `CreateAllocationFunctionDecl` lambda, rather than waiting 
until the second time we want it.


https://reviews.llvm.org/D33366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D9#759146, @hubert.reinterpretcast wrote:

> The `check-all` target passes even if the ellipsis-after-declarator-id 
> disambiguation as a declarator is removed entirely.


[...]

> So, on the whole, the stray ellipsis treatment is both too complicated and 
> not complicated enough.

I think if we want to keep it, the way to do that would be to carry on through 
the disambiguation process and treat it as a tiebreaker (that's what we do, for 
instance, if we see an undeclared identifier in a position where we're looking 
for a type). I'm not convinced that's worthwhile, especially since our existing 
testcases do not need this disambiguation rule, but perhaps we could remove the 
stray ellipsis treatment entirely for now and reconsider adding it back if we 
find poor diagnostics result from it later?


https://reviews.llvm.org/D9



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/Parse/Parser.h:1956-1957
   AccessSpecifier AS, DeclSpecContext DSC);
-  void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl);
+  void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl,
+ bool IgnorePrevDef = false);
   void ParseStructUnionBody(SourceLocation StartLoc, unsigned TagType,

Please remove this (now unused) parameter.



Comment at: lib/Parse/ParseDecl.cpp:4391
 if (TryConsumeToken(tok::equal, EqualLoc)) {
-  AssignedVal = ParseConstantExpression();
+  AssignedVal = ParseConstantExpression(NotTypeCast /*isTypeCast*/);
   if (AssignedVal.isInvalid())

Please revert this no-op change (left over from when you were passing in 
IgnorePrevDef).



Comment at: lib/Sema/SemaDecl.cpp:15423
+ isa(PrevDecl) &&
+ PrevDecl->isFromASTFile() && !isVisible(PrevDecl);
 // When in C++, we may get a TagDecl with the same name; in this case the

The `isFromASTFile` check here is not correct: whether a declaration is from an 
AST file is independent of whether it might be not visible due to being in a 
module (local submodule visibility rules allow a local declaration to not be 
visible). The `Modules` check is also not correct: we do visibility checking 
for local `#include`s of module headers under 
`-fmodules-local-submodule-visibility -fno-modules`.

We also shouldn't diagnose ambiguity if we find two different hidden previous 
declarations, but we will do so (within `LookupSingleName`) with this approach.

Digging into this a bit more, I think the root of the problem is that the 
`ND->isExternallyVisible()` call in `LookupResult::isHiddenDeclarationVisible` 
is wrong. Redeclaration lookup should never find hidden enumerators in C, 
because they do not have linkage (C11 6.2.2/6). (The same is true in C++, but I 
don't know whether we can apply the same thing there too, due to the different 
merging rules.) A function `foo` in some source file should not conflict with 
an enumerator `foo` in a non-imported module, for instance.


https://reviews.llvm.org/D31778



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Should I assume our "misplaced ellipsis" diagnostic requires that we 
disambiguate the ill-formed ellipsis-after-declarator-id as a declaration in 
some cases? If so, do we have tests for that somewhere?




Comment at: include/clang/Parse/Parser.h:2138
+  TPResult TryParseDeclarator(bool mayBeAbstract, bool mayHaveIdentifier = 
true,
+  bool mayHaveTrailingStrayEllipsis = true);
   TPResult

Given that this flag is enabling parsing of things that are *not* valid 
declarators, I think the default should be the other way around. If some 
calling code believes it's safe to parse a trailing ellipsis as part of a 
declarator, it should be explicitly opting into that.



Comment at: lib/Parse/ParseTentative.cpp:944
+(mayHaveTrailingStrayEllipsis ||
+ !(NextToken().is(tok::r_paren) || NextToken().is(tok::comma
   ConsumeToken();

hubert.reinterpretcast wrote:
> The check for what qualifies as "trailing" is not the strongest, but I find 
> it to be the most straightforward change that should do the job. One 
> alternative is to track whether an ellipsis was consumed within the current 
> loop iteration.
Use `!NextToken().isOneOf(tok::r_paren, tok::comma)` here.


https://reviews.llvm.org/D9



___
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.

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.

Some comments, but I'm happy for you to go ahead and commit after addressing 
them. Thanks!




Comment at: include/clang/Lex/Preprocessor.h:2004
+  ArrayRef getPreambleConditionalStack() const
+  { return PreambleConditionalStack.getStack(); }
+

Please put the `{` on the previous line and the `}` on the next line. We don't 
use this "sandwich braces" style in clang except when defining the function on 
the same line as the declaration.



Comment at: include/clang/Lex/PreprocessorLexer.h:182
+  void setConditionalLevels(ArrayRef CL)
+  {
+ConditionalStack.clear();

`{` on the previous line, please.



Comment at: lib/Lex/Lexer.cpp:2548
+  if (PP->isRecordingPreamble()) {
+PP->setRecordedPreambleConditionalStack(ConditionalStack);
+if (PP->isInPreamble())

This should be in the `isInPreamble()` conditional below, too. We don't want to 
make a redundant copy of the `ConditionalStack` at the end of each `#include`d 
file, just at the end of the overall preamble.



Comment at: lib/Lex/PPLexerChange.cpp:49
 
+bool Preprocessor::isInPreamble() const {
+  if (IsFileLexer())

I think this would be better named as `isInMainFile`: we don't care whether 
we're in the preamble, or even if there is one, here (the caller checks that).


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] D33207: Fix an assertion failure in FormatASTNodeDiagnosticArgument.

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


https://reviews.llvm.org/D33207



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Parse/ParseExpr.cpp:203-222
+  // Create the ExpressionEvaluationContext on the stack - but only if asked 
to.
+  struct EnterExpressionEvaluationContextConditionalRAII {
+llvm::AlignedCharArray
+MyStackStorage;
+const EnterExpressionEvaluationContext *const ConstantEvaluatedContext;
+EnterExpressionEvaluationContextConditionalRAII(bool CreateIt,

This seems more complexity than we need. How about factoring out a 
`ParseConstantExpressionInExprEvalContext` function that doesn't create a 
context, and then calling it from this function after creating the context?



Comment at: lib/Parse/ParseTemplate.cpp:1208-1233
   if (isCXXTypeId(TypeIdAsTemplateArgument)) {
 SourceLocation Loc = Tok.getLocation();
-TypeResult TypeArg = ParseTypeName(/*Range=*/nullptr,
-   Declarator::TemplateTypeArgContext);
+TypeResult TypeArg =
+ParseTypeName(/*Range=*/nullptr, Declarator::TemplateTypeArgContext);
 if (TypeArg.isInvalid())
   return ParsedTemplateArgument();
+

There's a bunch of whitespace changes here. I have no objection to them but 
they should be handled separately rather than mixed into this change.


https://reviews.llvm.org/D31588



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: test/SemaCXX/warn-shadow.cpp:214
+void handleLinkageSpec() {
+  typedef void externC; // expected-warning {{declaration shadows a typedef in 
linkage specification}}
+}

We should be producing a diagnostic talking about the enclosing namespace / TU 
scope, not the linkage specification. Whoever is emitting the diagnostic seems 
to be missing a call to `getRedeclContext()` -- although perhaps we could do 
that in the diagnostic code instead.


https://reviews.llvm.org/D33207



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32178: Delete unstable integration tests

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

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


https://reviews.llvm.org/D32178



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

I'd like to suggest an alternative design: don't add a new attribute., and 
instead change the semantics of `__attribute__((overloadable))` to permit at 
most one non-overloadable function in an overload set. That one function would 
implicitly get the `transparently_overloadable` semantics: it can be overloaded 
by `overloadable` functions and doesn't get a mangled name. This is basically 
exactly how extern "C" and extern "C++" functions overload in C++: you can have 
up to one extern "C" function plus as many (overloadable) C++ functions as you 
like.

Does that seem reasonable? Or is there some reason we need or want an explicit 
attribute for this case?


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D33013



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/D16171#540261, @phabricss wrote:

>   ../include/sys/stat.h:16:15: error: cannot apply asm label to function 
> after its first use
>   hidden_proto (__fxstat)
>   ~~^
>   ./../include/libc-symbols.h:420:19: note: expanded from macro 'hidden_proto'
> __hidden_proto (name, , __GI_##name, ##attrs)
> ^
>   ./../include/libc-symbols.h:424:33: note: expanded from macro 
> '__hidden_proto'
> extern thread __typeof (name) name __asm__ (__hidden_asmname (#internal)) 
> \
>


This patch does nothing about that error. This patch is dealing with a case 
where two distinct asm labels are provided for the same declaration, which 
would simply be a bug in the code. Given that we are not seeing that error any 
more, and that nicholas suggests that the reason is that the glibc maintainers 
applied a patch to fix it, this change seems unnecessary and deleterious.

As for the above change, we could theoretically accept such shenanigans, but it 
would require a much more invasive approach than the one in this patch -- in 
particular, it would require rewriting any code we've already emitted using the 
old name, changing it to use the new name instead. And even then, that is 
fundamentally not possible for some use cases of Clang (where we might have 
already, say, created machine code and started running it, possibly on a 
different machine, before we reach this asm label).

I would strongly suggest pushing back hard on the glibc maintainers and 
suggesting they don't rely on this working.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4275-4276
   "use of %0 with tag type that does not match previous declaration">;
+def warn_different_asm_label : Warning<"conflicting asm label">,
+InGroup;
 def warn_struct_class_tag_mismatch : Warning<

Move this earlier so that it's adjacent to the error form.

Please also use a different diagnostic group. This makes no sense in 
`-Wmismatched-tags`. Please also make this `DefaultError`; allowing it to be 
turned off might be OK, but this code pattern is still horribly broken, and we 
should not accept it by default.



Comment at: lib/Sema/SemaDecl.cpp:2388
 // This redeclaration changes __asm__ label.
-Diag(New->getLocation(), diag::err_different_asm_label);
+if (New->isUsed())
+  Diag(New->getLocation(), diag::err_different_asm_label);

This looks wrong. How could `New` already be used here, since it's new? Should 
this say `Old` instead?

Please also make sure we have test coverage for this. None of the tests below 
use the declaration before adding the conflicting label.


https://reviews.llvm.org/D16171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Er, please ignore the inline review comments; those predated the realisation 
that this doesn't actually fix the glibc build problem.


https://reviews.llvm.org/D16171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Thank you, some of these test typos are ... alarming. =)

A couple of the test updates don't look quite right, but this mostly looks 
great.




Comment at: test/Driver/amdgpu-features.c:1
-// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=0.0 %s -o 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-0-0 %s

This should say `-o -`



Comment at: test/Driver/amdgpu-features.c:5
 
-// RUN: %clang -### -target amdgcn -x cl -S -emit-llvm -mcpu=kaveri 
-mamdgpu-debugger-abi=1.0 %s -o 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s

Likewise.



Comment at: test/Driver/split-debug.h:6-13
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -fmodules 
-emit-module -fno-implicit-modules -fno-implicit-module-maps -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
 //
 // FIXME: This should fail using clang, except that the type of the output for
 // an object output with modules is given as clang::driver::types::TY_PCH
 // rather than TY_Object.
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -fmodules 
-### %s 2> %t

These parts of the test don't make sense: the `-fmodule-format=obj` and 
`-emit-module` are `-cc1` options, so testing how the driver deals with them 
doesn't really make a lot of sense. I would suggest deleting the highlighted 
region of this test rather than making it test the same thing three times.


https://reviews.llvm.org/D33013



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Please first check in a change that just regenerates cxx_dr_status without your 
changes, to separate out the changes due to the new issues list from the 
changes due to this patch. (You can go ahead and do that with no review.)


https://reviews.llvm.org/D32984



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



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

rsmith wrote:
> Typo 'abd'
Typo 'compatibale' =)



Comment at: lib/Parse/ParseDecl.cpp:4307
+Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
+//ParseEnumBody(StartLoc, D);
+ParseEnumBody(StartLoc, D,

Commented-out code, please remove.



Comment at: lib/Parse/ParseDecl.cpp:4313-4321
+if (SkipBody.CheckSameAsPrevious) {
+  if (!Actions.hasStructuralCompatLayout(TagDecl, SkipBody.New)) {
+// Bail out.
+DS.SetTypeSpecError();
+return;
+  }
+  // Make the previous decl visible.

The parser shouldn't be doing this itself, please move this to a function on 
`Sema` (`ActOnDuplicateDefinition` or something) and call that from here.



Comment at: lib/Sema/SemaExpr.cpp:2198-2209
+  if (R.isAmbiguous()) {
+if (!IgnorePrevDef)
+  return ExprError();
+// We already know that there's a hidden decl included in the lookup,
+// ignore it and only use the first found (the local) one...
+auto RF = R.makeFilter();
+NamedDecl *ND = R.getRepresentativeDecl();

bruno wrote:
> rsmith wrote:
> > This is gross. In order to make this work, you'd need to propagate the 
> > `IgnorePrevDef` flag through the entire expression parsing machinery.
> > 
> > Instead of this, how about deferring making the old definition visible 
> > until after you complete parsing the new one and do the structural 
> > comparison?
> Thanks for pointing it out, I totally missed this approach. Your suggestion 
> works and I'll change the patch accordingly. However, `IgnorePrevDef` still 
> needs to be threaded in `ParseEnumBody` and `ActOnEnumConstant` in order to 
> prevent the latter to emit `err_redefinition_of_enumerator`-like diagnostics.
I think the problem is that we don't take module visibility into account when 
doing redefinition checking for enumerators. Instead of passing through this 
flag, we should probably just ignore hidden declarations when checking for a 
redefinition of an enumerator.


https://reviews.llvm.org/D31778



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



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

`Decl` could also now be visible due to its lexically-enclosing context having 
a visible merged definition. I think this assertion should either be removed or 
changed to something like
```
assert(!isVisible(Decl) || );
```


https://reviews.llvm.org/D32828



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Serialization/ASTReaderDecl.cpp:213-215
+  assert(isa(D) ||
+ isa(D) &&
+ "Decl doesn't have specializations.");

Can this ever fail at runtime? I'd expect the below code to not compile if `D` 
isn't one of these types.



Comment at: lib/Serialization/ASTReaderDecl.cpp:3911-3924
+case UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION: {
+  // It will be added to the template's lazy specialization set when 
loaded.
+  SmallVector SpecID;
+  SpecID.push_back(ReadDeclID());
+  if (auto *CTD = dyn_cast(D))
+AddLazySpecializations(CTD, SpecID);
+  else if (auto *FTD = dyn_cast(D))

This will end up being quadratic time and using quadratic storage if a module 
adds N specializations to a template; instead, please move the SmallVector out 
of the loop and call `AddLazySpecializations` once after reading all the update 
records for the declaration. (It's probably best to move it all the way out to 
`loadDeclUpdateRecords`, in case we have a large number of modules each adding 
some specializations -- eg, if module A imports std::vector and declares A, 
module B declares B and uses vector, module C declares C and uses vector, 
..., we again should avoid quadratic behavior.)


https://reviews.llvm.org/D29951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

This makes a lot of sense to me. See also r302463: I think we probably want 
three levels of shadowing here: the main input shadows -fmodule-map-file, which 
shadows module maps loaded implicitly. (There's also a question of what we do 
with module map information loaded from an AST file: currently when that 
happens, we ignore the tokens for the parsed module map entirely. It might make 
more sense to have the module loaded from the AST file shadow the module from 
the module map, especially for an explicit module build, now that we have that 
functionality.)




Comment at: lib/Lex/ModuleMap.cpp:2574-2575
 
+  llvm::SaveAndRestore OldExplicit(CurrentModuleMapIsExplicitlyProvided);
+  CurrentModuleMapIsExplicitlyProvided |= IsExplicitlyProvided;
+

It would seem cleaner to make this a member of `ModuleMapParser` (and 
explicitly pass down the flag when parsing an `extern module` declaration). Is 
there a reason to use (essentially) global state for this?


https://reviews.llvm.org/D31269



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Let's go ahead with this. I've been unable to find a testcase that triggers the 
problem, but we shouldn't keep a known latent bug around just because we don't 
know how to expose it yet.


https://reviews.llvm.org/D32499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: test/CXX/drs/dr4xx.cpp:1205
 
-namespace dr496 { // dr496: no
+namespace dr496 { // dr496: reverted by dr2095 in 5.0
   struct A { int n; };

Write this as "dr496: sup 2094" and then rerun the `make_cxx_dr_status` script 
in www/ to regenerate cxx_dr_status.html



Comment at: test/CXX/drs/dr4xx.cpp:1209
   int check1[ __is_trivially_copyable(const int) ? 1 : -1];
-  int check2[!__is_trivially_copyable(volatile int) ? 1 : -1];
+  // FIXME: This checks the dr2095 behavior, not dr496
+  int check2[ __is_trivially_copyable(volatile int) ? 1 : -1];

2095 -> 2094? Also, why FIXME?


https://reviews.llvm.org/D32984



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Committed as r301822.




Comment at: lib/AST/ExprConstant.cpp:1301
 void addUnsizedArray(EvalInfo , QualType ElemTy) {
-  assert(Designator.Entries.empty() && getType(Base)->isPointerType());
-  assert(isBaseAnAllocSizeCall(Base) &&
- "Only alloc_size bases can have unsized arrays");
-  Designator.FirstEntryIsAnUnsizedArray = true;
   Designator.addUnsizedArrayUnchecked(ElemTy);
 }

rsmith wrote:
> We should call `checkSubobject` here, for cases like:
> 
> ```
> void f(int n) {
>   int arr[2][n];
>   constexpr int *p = [2][0];
> }
> ```
> 
> ... which we should reject because `arr[2]` is a one-past-the-end lvalue, so 
> cannot be indexed (we reject this if `n` is instead a constant).
Turns out this case is already failing before we get here: forming `arr[2]` 
requires determining the size of the array element type `int[n]`, which fails.


https://reviews.llvm.org/D32372



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks. Could you also add something like:

  struct A {};
  struct B : virtual A {};
  constexpr A  = (A&)*(B*)0;

to test/SemaCXX/constant-expression-cxx11.cpp to ensure we produce a suitable 
diagnostic, please?


https://reviews.llvm.org/D32675



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this looks good, just a couple of minor things and then it should be 
ready to land.

Do you have commit access or will you need someone else to commit this for you?




Comment at: lib/AST/ExprConstant.cpp:152
+   uint64_t , QualType , bool ,
+   bool ) {
 // This only accepts LValueBases from APValues, and APValues don't support

Please start variable names with an uppercase letter.



Comment at: lib/AST/ExprConstant.cpp:168
+  ArraySize = 0;
+  isUnsizedArray = true;
+}

The other 'most derived' paths through here should set this back to `false` (a 
sized array in a field in an element of an unsized array does not give an 
unsized array).



Comment at: lib/AST/ExprConstant.cpp:1301
 void addUnsizedArray(EvalInfo , QualType ElemTy) {
-  assert(Designator.Entries.empty() && getType(Base)->isPointerType());
-  assert(isBaseAnAllocSizeCall(Base) &&
- "Only alloc_size bases can have unsized arrays");
-  Designator.FirstEntryIsAnUnsizedArray = true;
   Designator.addUnsizedArrayUnchecked(ElemTy);
 }

We should call `checkSubobject` here, for cases like:

```
void f(int n) {
  int arr[2][n];
  constexpr int *p = [2][0];
}
```

... which we should reject because `arr[2]` is a one-past-the-end lvalue, so 
cannot be indexed (we reject this if `n` is instead a constant).


https://reviews.llvm.org/D32372



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Yes, let's first revert back to the clang 4.0 behavior, then please mail 
cfe-dev to discuss what the right behavior should be.


Repository:
  rL LLVM

https://reviews.llvm.org/D32566



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


https://reviews.llvm.org/D32410



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: test/Preprocessor/macro_paste_commaext.c:4
+// In the following tests, note that the output is sensitive to the
+// whitespace *preceeding* the varargs argument, as well as to
+// interior whitespace. AFAIK, this is the only case where whitespace

Typo (too many 'e's in "preceding").


https://reviews.llvm.org/D30427



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

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

One way we could make the behavior fully consistent (and more 
backwards-compatible with pre-clang-4.0) would be to revert both this and 
https://reviews.llvm.org/D26663, and provide the information that 
https://reviews.llvm.org/D26663 wanted to expose by another set of interface 
functions. What do other people with an interest in the libclang interface 
think?


Repository:
  rL LLVM

https://reviews.llvm.org/D32566



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D32405



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



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

I believe this is unreachable: an `ObjCBoxedExpr` will always have pointer (or 
dependent) type; the former will be handled below and the latter should never 
be evaluated at all. (We might want a mode to recurse into non-dependent 
subexpressions of value-dependent expressions, but that should probably be a 
separate visitor.)



Comment at: lib/AST/ExprConstant.cpp:5487
+  }
+
+case EvalInfo::EM_ConstantExpression:

Add `LLVM_FALLTHROUGH;` here, please.



Comment at: lib/AST/ExprConstant.cpp:5492
+case EvalInfo::EM_OffsetFold:
+  return Success(E);
+}

As far as I can see, this (from the pre-existing code) is very wrong: the 
evaluation semantics of `ObjCBoxedExpr` are to evaluate the subexpression and 
then pass that to a certain Objective-C method to box the result. We can't just 
skip evaluating the subexpression! We miscompile this, for instance:

```
@interface NSNumber
+ (id)numberWithInt:(int)n;
@end

int n;
int m = (@(n++), 0);
```

... completely losing the increment of `n` because we incorrectly return 
`Success` here without evaluating the subexpression.

Plus returning `Success` here seems like it's never likely to be useful, since 
CGExprConstant can't actually emit an `APValue` of this form.

As a minimal fix that would still let us perform this weird evaluation, we 
could unconditionally call `EvaluateIgnoredValue` here prior to returning 
`Success(E)`.


https://reviews.llvm.org/D32410



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

If we're now catching integer overflow in more cases, please add some relevant 
testcases. If this is a pure refactoring that enables those additional 
diagnostics to be produced in future, then this is fine as-is. (For the benefit 
of other people looking at this, the test changes here are caused by a 
pre-existing bug: the notes for why a reference is not usable in a constant 
expression are only produced if we happen to produce diagnostics the first time 
we try to evaluate them.)

I have an unsubstantiated performance concern: we've seen this overflow 
checking having a visible effect on compile times in LNT before, but I'm happy 
to let you figure out whether there's anything else you can do to measure this 
before bouncing it off the LNT bots. (And any cases we're checking with this 
patch but not without it seem to be arbitrary oversights rather than something 
principled.)


https://reviews.llvm.org/D32412



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D32455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

This change looks like it introduces a regression itself: given

  template struct A {};
  template using B = A;
  B bi;

... requesting the template arguments for the type `B` changes from 
producing  `int` to producing `int*` with this patch, which seems to directly 
oppose the intentions of https://reviews.llvm.org/D26663.

If the intent of this libclang function is to request the template arguments of 
the type as written, this change is wrong. If the intent is to request the 
template arguments of the desugared type, then https://reviews.llvm.org/D26663 
is wrong. But either way, it seems that reversing the order of these checks 
causes us to produce inconsistent results. Another example of the inconsistency:

  template using C = T;

With this patch, requesting the template arguments for `C` gives `int` but 
requesting the template arguments of `C` gives `void` rather than 
`A`.


Repository:
  rL LLVM

https://reviews.llvm.org/D32348



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

The change in direction from diagnosing the lvalue-to-rvalue conversion to 
diagnosing the pointer arithmetic seems fine to me (and is likely a better 
approach overall), but this means we should now treat a designator referring to 
element 0 of an array of unknown / runtime bound as being valid. We have 
minimal support for this already for `__builtin_object_size` evaluation, which 
you should be able to generalize to support arbitrary designators ending with 
such a value.




Comment at: lib/AST/ExprConstant.cpp:2957
+  else
+return CompleteObject();
+}

This path fails without producing a diagnostic (a default-constructed 
`CompleteObject()` is an error return). I think we should instead treat this as 
a valid `CompleteObject` with a type that simply can't be further decomposed.



Comment at: lib/AST/ExprConstant.cpp:5559-5567
+  // If we're dealing with an array of non-constant bound, the expression is
+  // not a constant expression. Use the Designator's most derived type field,
+  // since we may cover addition with a flexible array member.
+  if (!Info.checkingPotentialConstantExpression() && Result.Designator.Invalid
+   && !Result.Designator.MostDerivedType.isNull()
+   && Info.Ctx.getAsArrayType(Result.Designator.MostDerivedType))
+CCEDiag(PExp, diag::note_constexpr_array_unknown_bound_arithmetic)

I think this should be handled in `SubobjectDesignator::adjustIndex` instead; 
there are other ways to get to the pointer arithmetic logic (such as array 
indexing and the implicit indexing we do in some builtins).



Comment at: lib/AST/ExprConstant.cpp:5679-5680
+Result.addArray(Info, E, CAT);
+  else
+Result.Designator.setInvalid();
+}

We should never set a designator invalid without issuing a diagnostic. If you 
want to defer the diagnostic until pointer arithmetic happens, you need to be 
able to represent that situation in a valid designator. Perhaps generalizing 
the existing support for `isMostDerivedAnUnsizedArray` would be a path forward.


https://reviews.llvm.org/D32372



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



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

Please add a comment here, something like:

isCXXTypeId might look up and annotate an identifier as an id-expression during 
disambiguation, so enter the appropriate context for a constant expression 
template argument before trying to disambiguate.



Comment at: lib/Parse/ParseTemplate.cpp:1214-1243
+} else {
+  // If we disambiguated as an expression that we identified as potentially
+  // not being odr-used (consistent with a template argument context), and
+  // annotated our token as that expression, then remove it from the
+  // MaybeODRUsedExprs so that it doesn't trigger a false error, since it
+  // would otherwise have been removed when completing processing of a
+  // constant expression.

... we shouldn't need to do any of this: instead, keep your 
`ExprEvaluationContext` alive through the call to `ParseConstantExpression`, 
and tell it to not create its own context in this case.



Comment at: lib/Parse/ParseTemplate.cpp:1234-1237
+// it for later since we can be certain that this expression would
+// eventually have been removed during ActOnConstantExpression called
+// from ParseConstantExpression when parsing the non-type template
+// argument below.  Eitherway, the appropriate checking for an

This doesn't seem right. If the template parameter is of reference type, the 
named entity should be considered to be odr-used. And likewise just because we 
stopped disambiguation after finding an id-expression that names a non-type, 
that does not imply that the overall template argument is the entity named by 
that id-expression. (Eg, consider `X`, where `F` is a functor whose 
`operator()` returns `this` -- that template argument should be considered to 
odr-use `F`.) But...


Repository:
  rL LLVM

https://reviews.llvm.org/D31588



___
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.

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM. I'd like to make sure we try to use something better than the first 
import location for a module (that location is especially meaningless under 
`-fmodules-local-submodule-visibility`), but I'm happy for that (the big 
comment) to be dealt with as a follow-on change, and all the other comments 
here are minor, so feel free to commit after addressing those.




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">;
 }

rsmith wrote:
> This note ends in the middle of a sentence.
... this note still ends in the middle of a sentence.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4584
+def note_use_ifdef_guards :
+  Note <"unguarded header; consider using #ifdef guards or #pragma once">;
 

Nit: we generally put the `Note<` on the prior line.



Comment at: lib/Sema/SemaDecl.cpp:3594
+notePreviousDefinition(Previous.getRepresentativeDecl()->getLocation(),
+ New->getLocation());
 return New->setInvalidDecl();

Reindent.



Comment at: lib/Sema/SemaDecl.cpp:3812
+
+if (!IncLoc.isInvalid()) {
+  if (Mod) {

Use `isValid` to avoid double-negative.



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()) {

bruno wrote:
> rsmith wrote:
> > 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`).
> I tried this, but AFAIU the Decls coming from non-modular headers are usually 
> hidden, and since it has not been merged, which happens in the testcase 
> because it's a var redefinition error, then we have no import location to get 
> information from. Do you have a synthetic testcase in mind that I can use? 
> I'm happy to follow up with that (here or in a next patch).
The `int c = 1;` testcase seems like it ought to be pretty rare (defining a 
variable in a header), and it might make more sense to say "hey, you probably 
didn't want a strong definition in a header at all" rather than pointing out 
the header was included twice.

Here's one pattern we've seen a few times that might be useful as a testcase:

== foo.h:
```
#ifndef FOO
#define FOO
#include "bar.h"
struct A {};
#endif
```

== bar.h:
```
#ifndef BAR
#define BAR
#include "foo.h"
#endif
```

== module.map:
```
module bar { header "bar.h" }
```

== x.c:
```
#include "foo.h"
```

[This results in us entering the FOO include guard, importing bar.h (including 
a definition of A) and then parsing another definition of A.]


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] D32378: Insert invariant.group.barrier for pointers comparisons

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:3066-3067
+} else { // Unsigned integers and pointers.
+  if (CGF.CGM.getCodeGenOpts().StrictVTablePointers &&
+  CGF.CGM.getCodeGenOpts().OptimizationLevel > 0) {
+// Based on comparisons of pointers to dynamic objects, the optimizer

I think we need to do this regardless of optimization level -- if we LTO 
together a -O0 translation unit with a -O2 translation unit, we still need this 
protection for the comparisons in the -O0 TU.

(IIRC we chose to make -fstrict-vtable-pointers an IR-level ABI break, but we 
shouldn't do the same thing for optimization level.)


https://reviews.llvm.org/D32378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D31856#733845, @efriedma wrote:

> We normally use stdint.h from the host C library, rather than our own 
> version; this file is only relevant in -ffreestanding mode.  So it should be 
> safe to change.


Agreed; r89237 (and nearby changes: r89221, r89224, r89226) are simply wrong. 
We cannot determine the correct types from the bitwidth alone; there may be 
multiple types with the relevant width and we must pick the right one. If we 
back out all of those changes and instead use the correct types as provided by 
`__*_TYPE__`, we should be able to remove the `__*_WIDTH__` macros too.


https://reviews.llvm.org/D31856



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



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

Is there a reason to recompute the type here rather than using `Output`?


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

This needs testcases (the one from your summary plus the ones in my comments 
above would be good).




Comment at: lib/AST/ExprConstant.cpp:2622
   // Next subobject is an array element.
-  const ConstantArrayType *CAT = Info.Ctx.getAsConstantArrayType(ObjType);
-  assert(CAT && "vla in literal type?");
+  uint64_t actualIndex;
+  const ArrayType *AT = Info.Ctx.getAsArrayType(ObjType); // non-null by 
assumption

I think this should be named `ArrayBound` or similar.



Comment at: lib/AST/ExprConstant.cpp:2625-2626
+  if (I == 0) {
+/* we're dealing with the complete array object, which may have been 
declared
+   without a bound */
+actualIndex = Sub.MostDerivedArraySize;

Use line comments, start with capital letter, end with period, please.



Comment at: lib/AST/ExprConstant.cpp:2627
+   without a bound */
+actualIndex = Sub.MostDerivedArraySize;
+assert(Sub.MostDerivedIsArrayElement && "Complete object is an array, 
but isn't?");

This will not be the correct bound in general. This field is the array size of 
the *innermost* non-base-class subobject described by the designator, not the 
outermost one.

You could compute the correct bound here by going back to the `ValueDecl*` in 
the `CompleteObject` and checking its most recent declaration, but I think it 
would be preferable to do that when computing the type in `findCompleteObject` 
instead.

(It might seem possible to map to the most recent `VarDecl` when evaluating the 
`DeclRefExpr` instead, but that actually won't work in general, for cases like:

```
extern int arr[];
constexpr int *p = arr; // computes APValue referring to first declaration of 
'arr'
int arr[3];
constexpr int *q = p + 1; // should be accepted
```

... but `findCompleteObject` will happen late enough.)



Comment at: lib/AST/ExprConstant.cpp:5652-5653
 return false;
-} else {
+}
+else {
   Result.set(SubExpr, Info.CurrentCall->Index);

Per LLVM coding style, `else` goes on the same line as `}`.



Comment at: lib/AST/ExprConstant.cpp:5666
+else if (auto decl = Result.Base.dyn_cast()) {
+  // make sure to consider the completed type.
+  if (auto CAT = Info.Ctx.getAsConstantArrayType(cast(

Comments should start with a capital letter.



Comment at: lib/AST/ExprConstant.cpp:5670-5673
+  else {
+Result.Designator.setInvalid();
+CCEDiag(SubExpr, diag::note_constexpr_array_unknown_bound_decay);
+  }

Please suppress this diagnostic when 
`Info.checkingPotentialConstantExpression()`, since the expression is still 
potentially a constant expression. Example:

```
extern int arr[];
constexpr int *f() { return arr + 3; }
int arr[5];
constexpr int *p = f();
```

Here, when we check that the body of `f()` is valid, we need to ignore the fact 
that we don't know the array bound, since it could become known later.



Comment at: lib/AST/ExprConstant.cpp:5676
 else
   Result.Designator.setInvalid();
 return true;

We should produce a `CCEDiag` along this path too. (This happens for VLAs.)


https://reviews.llvm.org/D32372



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Please add some test coverage for these triples.


Repository:
  rL LLVM

https://reviews.llvm.org/D32269



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

This is sadly not a correct change. The relevant requirements (C11 7.20.3/2) on 
these macros are:

> Each instance of these macros shall be replaced by a constant expression 
> suitable for use in `#if` preprocessing directives, and this expression shall 
> have the same type as would an expression that is an object of the 
> corresponding type converted according to the integer promotions.

The "suitable for use in `#if`" requirement means that you cannot use a cast, 
and must instead use a suitable numeric suffix.

Can we instead use `__SIZE_MAX__`? (And likewise for `ptrdiff_t` etc.)




Comment at: clang/test/Preprocessor/stdint.c:1411
 // JOIN:PTRDIFF_MAX_ 2147483647
-// JOIN:SIZE_MAX_ 4294967295U
+// JOIN:SIZE_MAX_ ((unsigned a)4294967295U)
 // JOIN:INTMAX_MIN_ (-9223372036854775807LL -1)

Do we really define `__SIZE_TYPE__` to `unsigned a` for some target?


https://reviews.llvm.org/D31856



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

I'm not thrilled about adding yet more predefined macros, but it really doesn't 
make sense for libc++ to depend on `__GCC_*` macros when targeting Windows, nor 
for these to be Windows-only, so let's do it.




Comment at: lib/Frontend/InitPreprocessor.cpp:910-912
+  addLockFreeMacros("__CLANG_ATOMIC");
+  if (!LangOpts.MSVCCompat)
+addLockFreeMacros("__GCC_ATOMIC");

I'd sink the `_` into the prefix too, but this looks fine either way.


https://reviews.llvm.org/D32265



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D32199#732189, @hfinkel wrote:

> In https://reviews.llvm.org/D32199#731472, @rsmith wrote:
>
> > 1. C's "effective type" rule allows writes to set the type pretty much 
> > unconditionally, unless the storage is for a variable with a declared type
>
>
> To come back to this point: We don't really implement these rules now, and it 
> is not clear that we will. The problem here is that, if we take the 
> specification literally, then we can't use our current TBAA at all. The 
> problem is that if we have:
>
>   write x, !tbaa "int"
>   read x, !tbaa "int"
>   write x, !tbaa "float"
>   
>
> TBAA will currently tell us that the "float" write aliases with neither the 
> preceding read nor the preceding write.


Right, C's TBAA rules do not (in general) permit a store to be reordered before 
a memory operation of a different type, they only allow loads to be moved 
before stores. (Put another way, they do not tell you that pointers point to 
distinct memory locations, just that a stored value cannot be observed by a 
load of a different type.) You get the more general "distinct memory locations" 
result only for objects of a declared type.

C++ is similar, except that (because object lifetimes do not currently begin 
magically due to a store) you /can/ reorder stores past a memory operation of a 
different type if you know no object's lifetime began in between. (But 
currently we do not record all lifetime events in IR, so we can't do that 
today. Also, we may be about to lose the property that you can statically 
determine a small number of places that might start an object lifetime.)

> Also, a strict reading of C's access rules seems to rule out the premise 
> underlying our struct-path TBAA entirely. So long as I'm accessing a value 
> using a struct that has some member, including recursively, with that type, 
> then it's fine. The matching of the relative offsets is a sufficient, but not 
> necessary, condition for well-defined access. C++ has essentially the same 
> language (and, thus, potentially the same problem).

I agree this rule is garbage, but it's not as permissive as I think you're 
suggesting. The rule says that you can use an lvalue of struct type to access 
memory of struct field type. In C this happens during struct assignment, for 
instance. It does *not* permit using an lvalue of struct field type to access 
unrelated fields of the same struct. So C appears to allow this nonsense:

  char *p = malloc(8);
  *(int*)p = 0;
  *(int*)(p + 4) = 0;
  struct S {int n; float f;} s = *(struct S*)p; // use lvalue of type `struct 
S` to access object of effective type `int`, to initialize a `float`

but not this nonsense:

  float q = ((struct S*)p)->f; // ub, cannot use lvalue of type `float` to 
access object of effective type `int`

... which just means that we can't make much use of TBAA when emitting struct 
copies in C.

In C++, on the other hand, the rule is even more garbage, since there is no way 
to perform a memory access with a glvalue of class type. (The closest you get 
is that a defaulted union construction/assignment copies the object 
representation, but that's expressed in terms of copying a sequence of unsigned 
chars, and in any case those are member functions and so already require an 
object of the correct type to exist.) See wg21.link/cwg2051

> While I'd like the sanitizer to follow the rules, that's really useful only 
> to the extent that the compilers follow the rules. If the compilers are 
> making stronger assumptions, then I think that the sanitizer should also.

I agree.

>> If we want to follow the relevant language rules by default, that would 
>> suggest that "writes always set the type" should be enabled by default in C 
>> and disabled by default in C++. That may not be the right decision for other 
>> reasons, though. In C++, writes through union members and new-expressions 
>> should probably (re)set the type (do you have intrinsics the frontend can 
>> use to do so?).
> 
> Also, in C, memcpy gets to copy the type for a destination that does not have 
> declared types. It looks like the same is true for C++ for trivially-copyable 
> types. Is the first read/write sets the unknown type (i.e. memory from 
> malloc/calloc/memset, etc.) correct for C++ also?

As I recall, "store can create an object" is the broad direction that SG12 
agreed on for the cases where you have a pointer into a raw storage buffer 
(that is, a char array), and we want the low-level storage allocation functions 
to give us such a buffer.


https://reviews.llvm.org/D32199



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

> As I've currently implemented it, both reads and writes set the type of 
> previously-unknown storage, and after that it says fixed (unless you memcpy 
> to it, memset it, or its lifetime ends (the type gets reset on 
> lifetime.start/end and for malloc/allocas/etc.). There's a flag to enable the 
> "writes always set the type" rule, but that's not the default. Is this too 
> strict?

That seems like it will have at least three flavors of false positive:

1. C's "effective type" rule allows writes to set the type pretty much 
unconditionally, unless the storage is for a variable with a declared type
2. After a placement new in C++, you should be able to use the storage as a new 
type
3. Storing directly to a member access on a union (ie, with the syntax `x.a = 
b`) in C++ permits using the storage as the new type

If we want to follow the relevant language rules by default, that would suggest 
that "writes always set the type" should be enabled by default in C and 
disabled by default in C++. That may not be the right decision for other 
reasons, though. In C++, writes through union members and new-expressions 
should probably (re)set the type (do you have intrinsics the frontend can use 
to do so?).


https://reviews.llvm.org/D32199



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



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

This should probably go after all the standard orderings.



Comment at: lib/Sema/SemaOverload.cpp:3872
+  // promoted underlying type, if the two are different.
+  if (S.getLangOpts().CPlusPlus11 &&
+  (SCS1.getToType(1).getTypePtr() != SCS2.getToType(1).getTypePtr())) {

I don't think it makes sense to condition this on C++11 mode: we support fixed 
underlying types for enums in C++98 as an extension, and they should get this 
behavior too.



Comment at: lib/Sema/SemaOverload.cpp:3874-3879
+bool S1ConvResult = isEnumConversionToUnderlyingType(S.Context, SCS1);
+bool S2ConvResult = isEnumConversionToUnderlyingType(S.Context, SCS2);
+if (S1ConvResult && !S2ConvResult)
+  return ImplicitConversionSequence::Better;
+else if (!S1ConvResult && S2ConvResult)
+  return ImplicitConversionSequence::Worse;

This does not check that the "other" conversion converts to the promoted 
underlying type. It looks like this would (incorrectly) also order a case like:

```
enum A : char { a };
void f(char);
void f(const char&);
void g() { f(a); }
```


https://reviews.llvm.org/D32251



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



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

Typo 'abd'



Comment at: lib/Parse/ParseDecl.cpp:4236-4237
 
   Sema::SkipBodyInfo SkipBody;
+  Sema::CheckCompatTagInfo CheckCompatTag;
   if (!Name && TUK == Sema::TUK_Definition && Tok.is(tok::l_brace) &&

Do we really need both of these? The new stuff seems to be a natural extension 
of `SkipBodyInfo`, to say "parse it, check it's the same as the previous 
definition, then throw the new one away".



Comment at: lib/Sema/SemaDecl.cpp:12929
+  /// here is passed back to the parser, allowing the tag body to be parsed.
+  auto createTagFromNewDecl = [&](void) -> TagDecl * {
+assert(!getLangOpts().CPlusPlus && "not meant for C++ usage");

No `void` here, please.



Comment at: lib/Sema/SemaDecl.cpp:12962
+  // It is important for implementing the correct semantics that this
+  // happen here (in act on tag decl). The #pragma pack stack is
+  // maintained as a result of parser callbacks which can occur at

"act on tag decl" -> ActOnTag?



Comment at: lib/Sema/SemaDecl.cpp:13383-13384
+  // Allow ODR for ObjC/C in the sense that we won't keep more that
+  // one definition around (merge them). However, ensure the decl
+  // pass the structural compatibility check in C11 6.2.7/1.
+  bool AllowODR = getLangOpts().CPlusPlus || getLangOpts().ObjC1 ||

Plurality mismatch in "decl pass"



Comment at: lib/Sema/SemaExpr.cpp:2198-2209
+  if (R.isAmbiguous()) {
+if (!IgnorePrevDef)
+  return ExprError();
+// We already know that there's a hidden decl included in the lookup,
+// ignore it and only use the first found (the local) one...
+auto RF = R.makeFilter();
+NamedDecl *ND = R.getRepresentativeDecl();

This is gross. In order to make this work, you'd need to propagate the 
`IgnorePrevDef` flag through the entire expression parsing machinery.

Instead of this, how about deferring making the old definition visible until 
after you complete parsing the new one and do the structural comparison?


https://reviews.llvm.org/D31778



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

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

I think calling it a type aliasing sanitizer would somewhat conflate the 
details of the mechanism with the fundamentals of the check itself. For example:

  variant v;
  int  = v.get;
  v = 1.3f;
  int m = n;

... is a lifetime bug, not an aliasing bug, but would be caught by this check 
just the same. I'd be tempted to suggest EffectiveTypeSanitizer, since we seem 
to be more-or-less directly implementing C's effective type rules, except that 
name isn't so good for the C++ case. And in the longer term we will probably 
want to provide an option to enforce the real C++ lifetime rules whereby a 
store with certain !tbaa metadata is not sufficient to change the type of 
storage.

> One potential concern with calling it the type sanitizer is that we have an 
> abbreviation overlap with the thread sanitizer.

Perhaps we could abbreviate it as "tysan"? *shrug*


https://reviews.llvm.org/D32199



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

I don't like calling this a "TBAA sanitizer". What we're sanitizing is the 
object model and effective type rules; it seems irrelevant which specific 
compiler analysis passes would result in your program misbehaving if you break 
the rules. I would also expect that we will extend this in future to assign 
types to storage even in cases where there is no store (for instance, we should 
be able to catch `float f() { int n; return *(float*) }` despite there being 
no TBAA violation in the naive IR).

How about renaming this to something more like `-fsanitize=type`?


https://reviews.llvm.org/D32199



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32092: Attribute inline

[Richard Smith via Phabricator via cfe-commits]

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] D32092: Attribute inline

[Richard Smith via Phabricator via cfe-commits]

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

[Richard Smith via Phabricator via cfe-commits]

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] D27263: Address of bitfield in anonymous struct doesn't error.

[Richard Smith via Phabricator via cfe-commits]

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] D27263: Address of bitfield in anonymous struct doesn't error.

[Richard Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL300264: Diagnose attempt to take address of bitfield members 
in anonymous structs. (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D27263?vs=79759=95222#toc

Repository:
  rL 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 = ::bitfield; // expected-error {{address of bit-field 
requested}}
+int S2::*anon_pf = ::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 = ::bitfield; // expected-error {{address of bit-field requested}}
+int S2::*anon_pf = ::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] D27546: [ASTReader] Sort RawComments before merging

[Richard Smith via Phabricator via cfe-commits]

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] D27604: [Driver] Add compiler option to generate a reproducer

[Richard Smith via Phabricator via cfe-commits]

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] D31781: [Modules] Allow local submodule visibility without c++

[Richard Smith via Phabricator via cfe-commits]

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] D29877: Warn about unused static file scope function template declarations.

[Richard Smith via Phabricator via cfe-commits]

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] D29877: Warn about unused static file scope function template declarations.

[Richard Smith via Phabricator via cfe-commits]

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] D31187: Fix removal of out-of-line definitions.

[Richard Smith via Phabricator via cfe-commits]

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] D31069: Don't warn about an unreachable fallthrough annotation in a template function

[Richard Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL298477: Suppress warning on unreachable 
[[clang::fallthrough]] within a template… (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D31069?vs=92104=92588#toc

Repository:
  rL 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 , int ) {
+bool checkFallThroughIntoBlock(const CFGBlock , int ,
+   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 , int ) {
+bool checkFallThroughIntoBlock(const CFGBlock , int ,
+   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;
@@ 

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

[Richard Smith via Phabricator via cfe-commits]

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] D30848: Implement DR 373 "Lookup on namespace qualified name in using-directive"

[Richard Smith via Phabricator via cfe-commits]

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] D31069: Don't warn about an unreachable fallthrough annotation in a template function

[Richard Smith via Phabricator via cfe-commits]

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] D31069: Don't warn about an unreachable fallthrough annotation in a template function

[Richard Smith via Phabricator via cfe-commits]

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] D29877: Warn about unused static file scope function template declarations.

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/Sema.cpp:472-477
+// If this is a function template, we should remove if it has no
+// specializations.
+if (FunctionTemplateDecl *Template = FD->getDescribedFunctionTemplate()) {
+  if (std::distance(Template->spec_begin(), Template->spec_end()))
+return true;
+}

The comment doesn't match the code: you're removing function templates if they 
/do/ have specializations. And I think we should probably be walking the list 
of specializations and considering the template to be used if any 
specialization is used. That would affect a case like:

```
template static void f() {}
template<> static void f() {}
```

... where the primary template is still unused despite having a specialization.



Comment at: lib/Sema/Sema.cpp:492
 
   if (const VarDecl *VD = dyn_cast(D)) {
 // If a variable usable in constant expressions is referenced,

Should we do the same thing for variable templates?



Comment at: lib/Sema/SemaDecl.cpp:1496
 return false;
+  // 'static operator' functions are defined in headers; don't warn.
+  if (FD->isOverloadedOperator() &&

Why? Defining a static operator in a header sounds like a bug to me.


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] D30848: Implement DR 373 "Lookup on namespace qualified name in using-directive"

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Functionally, this looks good. How do the diagnostics look in the case where 
lookup only finds a non-namespace name? Eg,

  struct A { struct B {}; };
  namespace X = A::B;


https://reviews.llvm.org/D30848



___
cfe-commits 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"

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



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

This should say "dr373: 5" to indicate the first version of Clang with the fix.



Comment at: www/cxx_dr_status.html:2282
 Lookup on namespace qualified name in using-directive
-No
+Yes
   

... and this file should be regenerated so it lists the version.


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] D26057: [coroutines] Add DependentCoawaitExpr and fix re-building CoroutineBodyStmt.

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/TreeTransform.h:6802
+  return getDerived().RebuildDependentCoawaitExpr(
+  E->getKeywordLoc(), Result.get(), E->getOperatorCoawaitLookup());
+}

EricWF wrote:
> rsmith wrote:
> > You need to transform the 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.)
> Ack. Fixed.
> 
> Do you have an example that doesn't use a function-local definition? Since 
> `operator co_await` cannot be defined locally.
It can't be defined locally, but it can be *declared* locally. Example:

```
template future f(T t) {
  future operator co_await(T);
  co_await t;
}
struct X {};
auto x = f(X{});
```

... which would require that `future operator co_await(X)` is defined 
somewhere in the program.


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] D26057: [coroutines] Add DependentCoawaitExpr and fix re-building CoroutineBodyStmt.

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: include/clang/Sema/ScopeInfo.h:138-140
+  /// \brief Whether this function has already built, or tried to build, the
+  /// the initial and final coroutine suspend points.
+  bool NeedsCoroutineSuspends : 1;

Is the comment here correct? It seems a slightly odd match for the member name.



Comment at: lib/Sema/SemaCoroutine.cpp:33
 /// function type.
 static QualType lookupPromiseType(Sema , const FunctionProtoType *FnType,
+  SourceLocation KwLoc,

rsmith wrote:
> EricWF wrote:
> > The changes to this function are all unrelated cleanup/improvements.
> Just go ahead and commit these separately then.
Please commit these changes separately if they're cleanups unrelated to the 
main purpose of the patch.



Comment at: lib/Sema/TreeTransform.h:6974-6975
+
+  // FIXME(EricWF): Remove this
+  assert(isa(LookupResult.get()) && "Expected lookup 
expr");
+

Remove this :) (This is already checked by the `cast` below.)


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] D30590: Don't assume cleanup emission preserves dominance in expr evaluation

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Hal and I discussed exactly the same problem (in the context of coroutines) on 
Saturday and came up with exactly the same approach :) I think this is the 
right direction.




Comment at: lib/CodeGen/CGExprComplex.cpp:204-206
+Scope.ensureDominatingValue();
+Scope.ensureDominatingValue();
+Scope.ForceCleanup();

I'm a little concerned about the loose connection between 
`ensureDominatingValue` and `ForrceCleanup` here -- if you forget the 
`ForceCleanup`, you get silent misbehavior. How about removing 
`ensureDominatingValue` and instead passing a `std::initializer_list` 
to `ForceCleanup`?



Comment at: lib/CodeGen/CodeGenFunction.h:539
   private:
+SmallVector ValuesToReload;
 

If you keep a `SmallVector` here, set its inline size to 2 to avoid allocations 
for the `_Complex` case.


https://reviews.llvm.org/D30590



___
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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D29753#688834, @bruno wrote:

> > It seems to me that the problem here is that `DeclMustBeEmitted` is not 
> > safe to call in the middle of deserialization if anything 
> > partially-deserialized might be reachable from the declaration we're 
> > querying, and yet we're currently calling it in that case.
>
> `DeclMustBeEmitted` name seems misleading since we does more than just 
> checking, it actually tries to emit stuff.


It doesn't emit anything itself. But like most AST interactions, it *can* 
trigger more declarations being imported lazily from an external AST source, 
which can in turn result in them being passed to the AST consumer. And that's 
why the serialization code generally has to be careful to not call unbounded 
operations on the AST, because it doesn't want to reenter itself. But in this 
case we're violating that principle.

> If it's a local module, shouldn't be everything already available? Do we have 
> non-deserialized items for a local module? Maybe I get the wrong idea of what 
> local means in this context..

With this patch, we're calling `DeclMustBeEmitted` in the *non-local* module 
case (where there can be non-deserialized items).

>> 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?
> 
> `D->getImportedOwningModule` calls `isFromASTFile`, which refers to PCH and 
> modules, shouldn't it work for both then?

It only fixes the cases where `getImportedOwningModule` returns 0, which is the 
normal case for a PCH, but is rare for a declaration from a module.

> I couldn't come up with a testcase that triggered it for modules though.

Something like this triggers it for me:

  $ cat test/PCH/empty-def-fwd-struct.modulemap
  module M { header "empty-def-fwd-struct.h" }
  $ clang -cc1 -x c++ -std=c++14 -fmodules -emit-module -fmodule-name=M 
test/PCH/empty-def-fwd-struct.modulemap -o tmp.pcm
  $ cat use.cpp
  #include "test/PCH/empty-def-fwd-struct.h"
  $ clang -cc1 -x c++ -std=c++14 -fmodules -emit-llvm -fmodule-file=tmp.pcm 
use.cpp -o -
  clang: [...]/src/tools/clang/lib/AST/RecordLayoutBuilder.cpp:2933: const 
clang::ASTRecordLayout ::ASTContext::getASTRecordLayout(const 
clang::RecordDecl *) const: Assertion `D && "Cannot get layout of forward 
declarations!"' failed.




Comment at: test/PCH/empty-def-fwd-struct.h:2
+// RUN: %clang_cc1 -emit-pch -x c++-header %s -std=c++14 -o %t.pch
+// RUN: %clang_cc1 -emit-llvm -x c++ /dev/null -std=c++14 -include-pch %t.pch 
-o %t.o
+struct FVector;

Since you don't care about what's in the produced LLVM IR, you can use 
`-emit-llvm-only` instead here.


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] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

[Richard Smith via Phabricator via cfe-commits]

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

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

[Richard Smith via Phabricator via cfe-commits]

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] D20710: Lit C++11 Compatibility Patch #9

[Richard Smith via Phabricator via cfe-commits]

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] D29812: Update template-id-expr.cpp test to work when compiler defaults to non-C++03 standard

[Richard Smith via Phabricator via cfe-commits]

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] D30315: [Driver] Move architecture-specific free helper functions to their own files.

[Richard Smith via Phabricator via cfe-commits]

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] D30082: Fix assertion when generating debug information for deduced template specialization types.

[Richard Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL295794: Fix assertion failure when generating debug 
information for a variable (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D30082?vs=88943=89300#toc

Repository:
  rL LLVM

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] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

[Richard Smith via Phabricator via cfe-commits]

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.

[Richard Smith via Phabricator via cfe-commits]

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] D30082: Fix assertion when generating debug information for deduced template specialization types.

[Richard Smith via Phabricator via cfe-commits]

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] D29863: [libc++] Fix PR 31938 - std::basic_string constructors use non-deductible parameter types.

[Richard Smith via Phabricator via cfe-commits]

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] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

[Richard Smith via Phabricator via cfe-commits]

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] D28510: Reinstate CWG1607 restrictions on lambdas appearing inside certain constant-expressions

[Richard Smith via Phabricator via cfe-commits]

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] D29724: [Driver] Report available language standards on user error

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Committed as r295113.


https://reviews.llvm.org/D29724



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

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] D29748: [cxx1z-constexpr-lambda] Implement captures - thus completing implementation of constexpr lambdas.

[Richard Smith via Phabricator via cfe-commits]

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  }() ==  // 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] D29863: [libc++] Fix PR 31938 - std::basic_string constructors use non-deductible parameter types.

[Richard Smith via Phabricator via cfe-commits]

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] D29724: [Driver] Report available language standards on user error

[Richard Smith via Phabricator via cfe-commits]

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