[PATCH] D50055: Update the coding standard about NFC changes and whitespace

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

Thank you for the review and discussion -- I've commit in r339455.


https://reviews.llvm.org/D50055



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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Committed in r339456; if @delesley has concerns with the commit, they can be 
addressed post-commit. Thank you for the patch!


Repository:
  rC Clang

https://reviews.llvm.org/D49885



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

[PATCH] D50526: Model type attributes as regular Attrs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this -- the approach you've taken looks good, and I 
mostly have little nits here and there. I think this is a great refactoring 
overall though -- much needed!




Comment at: include/clang/AST/Type.h:1856
+
+  /// Was this type written with the special inert-in-MRC __unsafe_unretained
+  /// qualifier?

Typo: MRC should be ARC (typo was present in the original code).



Comment at: include/clang/Basic/Attr.td:1510
+  let Spellings = [Keyword<"__unsafe_unretained">];
+  let Documentation = [Undocumented];
+}

I don't suppose you can throw in some quick docs for this keyword? Or is this 
not really user-facing? If it's not user-facing, perhaps it should have no 
spellings and only ever be created implicitly?



Comment at: lib/AST/Type.cpp:1624
+  const Type *Cur = this;
+  while (auto *AT = Cur->getAs()) {
+if (AT->getAttrKind() == AK)

`const auto *`



Comment at: lib/AST/Type.cpp:3215
   }
-  llvm_unreachable("bad attributed type kind");
 }

Probably need to keep this to prevent MSVC from barking about not all control 
paths returning a value.



Comment at: lib/AST/Type.cpp:3653
+Optional
+Type::getNullability(const ASTContext &context) const {
   QualType type(this, 0);

Update the identifiers for coding standards since you're basically rewriting 
the function?



Comment at: lib/AST/Type.cpp:3655
   QualType type(this, 0);
-  do {
+  while (auto *AT = type->getAs()) {
 // Check whether this is an attributed type with nullability

`const auto *`



Comment at: lib/AST/TypeLoc.cpp:408
   if (auto attributedLoc = getAs()) {
-if (attributedLoc.getAttrKind() == AttributedType::attr_nullable ||
-attributedLoc.getAttrKind() == AttributedType::attr_nonnull ||
-attributedLoc.getAttrKind() == AttributedType::attr_null_unspecified)
-  return attributedLoc.getAttrNameLoc();
+auto *A = attributedLoc.getAttr();
+if (A && (isa(A) || isa(A) ||

Might as well go with `const Attr *` here, the `auto` gets us nothing.



Comment at: lib/Sema/SemaDecl.cpp:6024
   // by applying it to the function type.
-  if (ATL.getAttrKind() == AttributedType::attr_lifetimebound) {
+  if (auto *A = ATL.getAttrAs()) {
 const auto *MD = dyn_cast(FD);

`const auto *`



Comment at: lib/Sema/SemaType.cpp:178-180
+// their TypeLocs makes it hard to correctly assign these. We use the
+// keep the attributes in creation order as an attempt to make them
+// line up properly.

"We use the keep the attributes" isn't grammatical. Should it be, "We keep the 
attributes in creation order" instead?



Comment at: lib/Sema/SemaType.cpp:181
+// line up properly.
+using TypeAttr = std::pair;
+SmallVector TypeAttrs;

It's unfortunate to use the name `TypeAttr` here -- unqualified uses of the 
type name, like below, makes it look like it might be the `TypeAttr` from Attr.h



Comment at: lib/Sema/SemaType.cpp:3884
 
+template
+static AttrT *createSimpleAttr(ASTContext &Ctx, ParsedAttr &Attr) {

Did clang-format do this? It seems like it's not formatted how I'd expect.



Comment at: lib/Sema/SemaType.cpp:6360
+  }
+}
+

MSVC may complain about not all control paths returning a value here.



Comment at: lib/Serialization/ASTReaderDecl.cpp:2695
+  Attr *New = nullptr;
+  auto Kind = (attr::Kind)(V - 1);
+  SourceRange Range = Record.readSourceRange();

This could use a comment to explain why the -1 is required. Also, do we have a 
preference for C-style vs static_cast here?



Comment at: lib/Serialization/ASTReaderDecl.cpp:2707
 void ASTReader::ReadAttributes(ASTRecordReader &Record, AttrVec &Attrs) {
-  for (unsigned i = 0, e = Record.readInt(); i != e; ++i) {
-Attr *New = nullptr;
-auto Kind = (attr::Kind)Record.readInt();
-SourceRange Range = Record.readSourceRange();
-ASTContext &Context = getContext();
-
-#include "clang/Serialization/AttrPCHRead.inc"
-
-assert(New && "Unable to decode attribute?");
-Attrs.push_back(New);
-  }
+  for (unsigned i = 0, e = Record.readInt(); i != e; ++i)
+Attrs.push_back(Record.readAttr());

Identifiers don't meet coding style rules.



Comment at: lib/Serialization/ASTWriter.cpp:4485-4486
+  push_back(Attrs.size());
+  for (const auto *A : Attrs)
+AddAttr(A);
 }

`llvm::for_each(Attrs, AddAttr);` ?


Repository:
  rC Clang

https://reviews.llvm.org/D50526



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.or

[PATCH] D50467: [SEMA] add more -Wfloat-conversion to compound assigment analysis

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
This revision now requires changes to proceed.



Comment at: lib/Sema/SemaChecking.cpp:10424
+  // We don't want to warn for system macro.
+  S.SourceMgr.isInSystemMacro(E->getOperatorLoc()))
+// warn about dropping FP rank.

This looks incorrect to me -- this is testing the rank difference and that it 
is in a system macro (the `!` was dropped). If this didn't cause any tests to 
fail, we should add a test that would fail for it.


Repository:
  rC Clang

https://reviews.llvm.org/D50467



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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:14251-14253
+if (const auto *Ptr = dyn_cast(Ty))
+  Inner = Ptr->getPointeeType();
+else if (const auto *Arr = dyn_cast(Ty))

I don't think this strips off sugar from the type, so I suspect it won't 
properly handle a typedef to a pointer type, for instance, or a type including 
parens. You should add tests for these cases.


Repository:
  rC Clang

https://reviews.llvm.org/D49511



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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Oops, I hit "Send" too soon. I was going to say that you should also keep an 
eye on https://reviews.llvm.org/D50526 because that may impact this patch (or 
vice versa if this one lands first).


Repository:
  rC Clang

https://reviews.llvm.org/D49511



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

[PATCH] D50467: [SEMA] add more -Wfloat-conversion to compound assigment analysis

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:10424
+  // We don't want to warn for system macro.
+  S.SourceMgr.isInSystemMacro(E->getOperatorLoc()))
+// warn about dropping FP rank.

nickdesaulniers wrote:
> aaron.ballman wrote:
> > This looks incorrect to me -- this is testing the rank difference and that 
> > it is in a system macro (the `!` was dropped). If this didn't cause any 
> > tests to fail, we should add a test that would fail for it.
> Thanks for the code review!
> 
> Sorry, I don't understand what you mean by "rank difference"? F16 vs F32 vs 
> F64 vs F128 etc?
> 
> When you say "this looks incorrect," are you commenting on the code I added 
> (Lines 10415 to 10418) or the prexisting code that I moved (Lines 
> 10420-10427)?
> 
> Good catch on dropping the `!`, that was definitely not intentional (and now 
> I'm curious if `dw` in vim will delete `(!` together), will add back.
> 
> I'm happy to add a test for the system macro, but I also don't know what that 
> is.  Can you explain?
Sorry, my explanation may have been confusing. A better way to phrase it would 
have been "this isn't doing what the old code used to do, and I don't think you 
intended to change it." I was talking about the `!` that disappeared.

I think you can use linemarker directives to get this to happen:
```
# 1 "systemheader.h" 1 3
#define MACRO

# 1 "test_file.c" 2
// Your code that uses MACRO.
```


Repository:
  rC Clang

https://reviews.llvm.org/D50467



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

[PATCH] D50606: [ASTMatchers] Add matchers unresolvedMemberExpr, cxxDependentScopeMemberExpr

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM, but please split out the unrelated changes into their own commit.




Comment at: docs/LibASTMatchersReference.html:1168-1173
+MatcherStmt>imaginaryLiteralMatcherImaginaryLiteral>...
+Matches imaginary 
literals, which are based on integer and floating
+point literals e.g.: 1i, 1.0i
+
+
+

This change looks unrelated to your patch.



Comment at: docs/LibASTMatchersReference.html:1280
 }
-}
 

This change looks unrelated to your patch.


Repository:
  rC Clang

https://reviews.llvm.org/D50606



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

[PATCH] D50605: [ASTMatchers] Let hasAnyArgument also support CXXUnresolvedConstructExpr

[Aaron Ballman via Phabricator via cfe-commits]

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

Please be sure to regenerate the AST matcher documentation as well, as I'd 
expect to see some documentation changes from this.




Comment at: include/clang/AST/ExprCXX.h:3436
   using const_arg_iterator = const Expr* const *;
+  using arg_const_range = llvm::iterator_range;
 

Please name this `const_arg_range` for consistency.


Repository:
  rC Clang

https://reviews.llvm.org/D50605



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Aaron Ballman via Phabricator via cfe-commits]

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

Alex has had plenty of time to respond, so I'm fine handling any concerns he 
has post-commit. Do you need me to commit this on your behalf?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+  Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);

I think this needs a `not(isExpansionInSystemHeader())` in there, as this is 
going to trigger on code that includes a header file using an `absl` namespace. 
If I'm incorrect and users typically include abseil as something other than 
system includes, you'll have to find a different way to solve this.



Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:32
+   "namespace 'absl' is reserved for implementation of the Abseil library "
+   "and should not be opened in the user code");
+}

in the user code -> in user code

Does Abseil prohibit the user from specializing templates in the `absl` 
namespace with user-defined types?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50580



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

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+  // TODO(hugoeg): refactor matcher to be configurable or just match on any 
internal access from outside the enclosing namespace.
+  

JonasToth wrote:
> Nit: This comment is very long, pls break the line
Also, we don't add developer names to comments.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:39-41
+   "depends upon internal implementation details, which violates the "
+   "Abseil compatibilty guidelines. See "
+   "https://abseil.io/about/compatibility";);

Diagnostic text is not supposed to be grammatically correct with capitalization 
and punctuation. Please do not put links into diagnostics. It's more 
appropriate for this to be in the documentation. I think this might be improved 
wording: `do not reference the 'internal' namespace; those implementation 
details are reserved to Abeil` or something along those lines.



Comment at: clang-tidy/abseil/NoInternalDepsCheck.h:19-21
+/// Finds instances where the user depends on internal details and warns them
+/// against doing so.
+/// This check should not be run on internal Abseil files or Abseil source 
code.

Can you re-flow these comments?



Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:6
+
+Gives a warning if code using Abseil depends on internal details. If something 
is in a namespace or filename/path that includes the word “internal”, code is 
not allowed to depend upon it beaucse it’s an implementation detail. They 
cannot friend it, include it, you mention it or refer to it in any way. Doing 
so violtaes Abseil's compatibility guidelines and may result in breakage.
+

JonasToth wrote:
> s/violtaes/violates/
Please wrap lines to 80 cols.

Nothing in this check looks at filenames and paths, but this suggests the check 
will find those. Is that intended work that's missed in this patch, or are the 
docs incorrect?


https://reviews.llvm.org/D50542



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

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Thank you for the patch and great discussion! I've commit in r339516.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D50467: [SEMA] add more -Wfloat-conversion to compound assigment analysis

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D50467



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

[PATCH] D50526: Model type attributes as regular Attrs

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM! Thank you for this fantastic work!




Comment at: include/clang/Basic/Attr.td:1510
+  let Spellings = [Keyword<"__unsafe_unretained">];
+  let Documentation = [Undocumented];
+}

rsmith wrote:
> aaron.ballman wrote:
> > I don't suppose you can throw in some quick docs for this keyword? Or is 
> > this not really user-facing? If it's not user-facing, perhaps it should 
> > have no spellings and only ever be created implicitly?
> This isn't actually the primary representation of `__unsafe_unretained`, this 
> is an internal placeholder for "the user wrote `__unsafe_unretained` but 
> we're not in ARC mode so it's meaningless (hence "inert")". So I don't think 
> this is where we should attach the documentation (the right place is the 
> `ObjCOwnership` attribute, which is also currently `Undocumented`, sadly). 
> I'll at least add a comment to the .td file to explain that.
Ah, thank you for the explanation; I agree.



Comment at: lib/AST/Type.cpp:1624
+  const Type *Cur = this;
+  while (auto *AT = Cur->getAs()) {
+if (AT->getAttrKind() == AK)

rsmith wrote:
> aaron.ballman wrote:
> > `const auto *`
> Done, but...
> 
> The pointee type is deduced as `const` anyway, so the `const` doesn't give 
> any extra type safety. So the only benefit would be to the reader, and I 
> don't think a reader of this code would care whether `*AT` is mutable, so the 
> `const` seems like a distraction to me (and hence the explicit `const` is a 
> minor harm to readability rather than a minor improvement). I can see how a 
> reader trained to think that absence-of-`const` implies that mutation is 
> intended would find the explicit `const` clearer, but (for better or probably 
> worse) that's not the style we generally use in Clang (for instance, I didn't 
> mark the `AK` parameter as `const`, but there is no implication that I intend 
> to modify it).
> 
> That said, I don't feel strongly about this, and I certainly have no 
> objection to making the change here. If we generally want to move to a style 
> where `auto` is always accompanied by an explicit `const` when `const` would 
> be deduced (in much the same way that we already expect that `auto` be 
> accompanied by an explicit `*` when a pointer type would be deduced), I'd be 
> OK with that -- but we should discuss that somewhere more visible than this 
> review thread and include it in our general coding guidelines.
> That said, I don't feel strongly about this, and I certainly have no 
> objection to making the change here. If we generally want to move to a style 
> where auto is always accompanied by an explicit const when const would be 
> deduced (in much the same way that we already expect that auto be accompanied 
> by an explicit * when a pointer type would be deduced), I'd be OK with that 
> -- but we should discuss that somewhere more visible than this review thread 
> and include it in our general coding guidelines.

Yeah, I actually thought this already was part of the coding guidelines, truth 
be told. But I went and looked again and realized we only talk about `*` and 
`&` being deduced, not qualifiers. I guess my preference has always been to 
explicitly spell out pertinent information about the type beyond the name, such 
as whether it's `const`, whether it's a pointer, etc. Basically, things that 
aren't immediately obvious from the context.

I prefer being explicit about spelling out the `const` here because it makes it 
obvious that the type is not intended to be mutated, but I only really care 
about it in cases where the type is deduced to a pointer or reference (and so 
mutating operations might be hidden behind a function call that looks harmless).

Perhaps this is worth starting a coding guideline discussion over?



Comment at: lib/Sema/SemaType.cpp:3884
 
+template
+static AttrT *createSimpleAttr(ASTContext &Ctx, ParsedAttr &Attr) {

rsmith wrote:
> aaron.ballman wrote:
> > Did clang-format do this? It seems like it's not formatted how I'd expect.
> How would you expect it? clang-format puts a space after the `template` 
> keyword, unfortunately, and IIRC can't be configured to not do so. Though as 
> a consequence, it looks like the space is now more common in clang code by a 
> 2:1 ratio despite being "clearly wrong" ;-(
I was expecting to see a space after `template` as I thought that was the most 
common form of it in the code base. :-D



Comment at: lib/Sema/SemaType.cpp:6360
+  }
+}
+

rsmith wrote:
> aaron.ballman wrote:
> > MSVC may complain about not all control paths returning a value here.
> I'm confident that this pattern is fine; we use it all over the place. MSVC 
> knows that control flow cannot leave an `llvm_unreachable(...)`.
Yeah, I think I was mis-remembering a pattern t

[PATCH] D50214: Add inherited attributes before parsed attributes.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: rsmith, echristo, dlj, nicholas.
aaron.ballman edited reviewers, added: rsmith; removed: nicholas, dlj, echristo.
aaron.ballman added a comment.

In general, I think this is the right way to go. I've added @rsmith to the 
reviewers because I'm curious what his thoughts are on this approach, though.




Comment at: lib/AST/DeclBase.cpp:854-859
+  auto I = Attrs.begin(), E = Attrs.end();
+  for (; I != E; ++I) {
+if (!(*I)->isInherited())
+  break;
+  }
+  Attrs.insert(I, A);

The unfortunate part about this is that inherited attributes are fairly common, 
so this extra searching may happen more often than we'd like. I wonder how bad 
it would be to keep track of the location within the list where the inherited 
attributes start. Then again, the list of attributes should be relatively short 
in most cases, so this search isn't likely to be too expensive.

Having some performance measurements might help decide this, too.


Repository:
  rC Clang

https://reviews.llvm.org/D50214



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

[PATCH] D49910: [clang-tidy] Recognize [[clang::reinitializes]] attribute in bugprone-use-after-move

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM! An attribute really is the right way to go for this sort of thing, and it 
sounds like you've got a base of users looking for the functionality.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49910



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

[PATCH] D50605: [ASTMatchers] Let hasAnyArgument also support CXXUnresolvedConstructExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/ExprCXX.h:3436
   using const_arg_iterator = const Expr* const *;
+  using arg_const_range = llvm::iterator_range;
 

shuaiwang wrote:
> aaron.ballman wrote:
> > Please name this `const_arg_range` for consistency.
> `arg_const_range` is more widely in clang
`arg_const_range` makes no sense -- the range is of constant args, not an 
argument over a constant range. Also, it's weird to have `const_arg_iterator` 
with `arg_const_range` in the same class. I've cleaned up those few instances 
in r339527. Thanks for pointing this out!


Repository:
  rC Clang

https://reviews.llvm.org/D50605



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

[PATCH] D50605: [ASTMatchers] Let hasAnyArgument also support CXXUnresolvedConstructExpr

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D50605



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

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23
+
+  Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"),
+ this);

hokein wrote:
> aaron.ballman wrote:
> > I think this needs a `not(isExpansionInSystemHeader())` in there, as this 
> > is going to trigger on code that includes a header file using an `absl` 
> > namespace. If I'm incorrect and users typically include abseil as something 
> > other than system includes, you'll have to find a different way to solve 
> > this.
> Yeah, we have discussed this issue internally.  abseil-user code usually 
> includes abseil header like `#include "whatever/abseil/base/optimization.h"`, 
> so `IsExpansionInSystemHeader` doesn't work for most cases. 
> 
> We need a way to filter out all headers being a part of abseil library. I 
> think we can create a matcher `InExpansionInAbseilHeader` -- basically using 
> `IsExpansionInFileMatching` with a regex expression that matches typical 
> abseil include path. What do you think?
> 
> And we'll have more abseil checks (e.g.  `abseil-no-internal-deps`) that use 
> `InExpansionInAbseilHeader`, we should put it to a common header.
I think that is a sensible approach.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50580



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

[PATCH] D50647: [Sema] fix -Wfloat-conversion test case.

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM, though you don't need review for fixing bots.


Repository:
  rC Clang

https://reviews.llvm.org/D50647



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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:14249
+
+if (Sema::TypeHasNoDeref(Inner))
+  DeclRef = E;

The sugar was stripped off at the pointer level, but not at the pointee level. 
e.g.,
```
typedef int (bobble);
typedef bobble * (frobble);
typedef frobble * yobble;

yobble gobble;
```
I think you can handle this within `TypeHasNoDeref()` and that should fix up 
all the callers.


Repository:
  rC Clang

https://reviews.llvm.org/D49511



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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:14249
+
+if (Sema::TypeHasNoDeref(Inner))
+  DeclRef = E;

leonardchan wrote:
> aaron.ballman wrote:
> > The sugar was stripped off at the pointer level, but not at the pointee 
> > level. e.g.,
> > ```
> > typedef int (bobble);
> > typedef bobble * (frobble);
> > typedef frobble * yobble;
> > 
> > yobble gobble;
> > ```
> > I think you can handle this within `TypeHasNoDeref()` and that should fix 
> > up all the callers.
> So `TypeHasNoDeref()` checks for the attribute on the base type already and 
> is called after the pointer is stripped off. Attempting to desugar via 
> `getDesugaredType()` here also removes the `address_space` attribute from the 
> type I'm checking.
> 
> Do you know another method that essentially "expands" the typedefs without 
> stripping the qualifiers? Otherwise I think I do need do the desugaring at 
> the pointer level. Alternatively I could also change this method such that it 
> accepts pointers instead of pointees since it appears I already call 
> `getDesugaredType()` for almost every pointer who's pointee I'm passing to 
> `TypeHasNoDeref()`.
> 
> Also I tested with your example and the warning still seems to be thrown 
> appropriately. 
I think you have to do the desugaring manually in a loop with 
`getSingleStepDesugaredType()` so that you don't strip off attributed type 
information along with the rest of the type sugar.

> Also I tested with your example and the warning still seems to be thrown 
> appropriately.

The example may depend on where you put the attribute (inside the parens vs 
outside the parens, for instance); it was an off-the-cuff example, so it may 
need some tweaking.


Repository:
  rC Clang

https://reviews.llvm.org/D49511



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

[PATCH] D50214: Add inherited attributes before parsed attributes.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/DeclBase.cpp:854-859
+  auto I = Attrs.begin(), E = Attrs.end();
+  for (; I != E; ++I) {
+if (!(*I)->isInherited())
+  break;
+  }
+  Attrs.insert(I, A);

Meinersbur wrote:
> aaron.ballman wrote:
> > The unfortunate part about this is that inherited attributes are fairly 
> > common, so this extra searching may happen more often than we'd like. I 
> > wonder how bad it would be to keep track of the location within the list 
> > where the inherited attributes start. Then again, the list of attributes 
> > should be relatively short in most cases, so this search isn't likely to be 
> > too expensive.
> > 
> > Having some performance measurements might help decide this, too.
> Timed clang-compilation using perf (`perf stat bin/clang 
> llvm/unittests/IR/InstructionsTest.cpp ...`), before this patch (r339574):
> ```
>7647.415800  task-clock (msec) #0.983 CPUs utilized
>289  context-switches  #0.038 K/sec
> 26  cpu-migrations#0.003 K/sec
> 86,494  page-faults   #0.011 M/sec
> 19,068,741,863  cycles#2.493 GHz
> 26,581,355,844  instructions  #1.39  insn per cycle
>  6,242,394,037  branches  #  816.275 M/sec
>128,405,656  branch-misses #2.06% of all branches
> 
>7.779848330 seconds time elapsed
> ```
> 
> With this patch:
> ```
>7679.173467  task-clock (msec) #0.987 CPUs utilized
>321  context-switches  #0.042 K/sec
> 23  cpu-migrations#0.003 K/sec
> 86,071  page-faults   #0.011 M/sec
> 19,150,248,538  cycles#2.494 GHz
> 26,667,465,697  instructions  #1.39  insn per cycle
>  6,262,381,898  branches  #  815.502 M/sec
>128,527,669  branch-misses #2.05% of all branches
> 
>7.779477815 seconds time elapsed
> ```
> (Intel(R) Xeon(R) Platinum 8180M CPU @ 2.50GHz)
> 
> The vector insert operation is also be `O(n)`. If the number of non-inherited 
> is expected to be smaller, we can instead search for the first inherited 
> attribute starting at the end of the vector. If we want to avoid the `O(n)` 
> entirely, we need one list for inherited and another for non-inherited 
> attributes.
Thank you for gathering this data -- those perf numbers look reasonable to me. 
We can refactor for performance later if it ever turns up on the hot path.


Repository:
  rC Clang

https://reviews.llvm.org/D50214



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

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

[Aaron Ballman via Phabricator via cfe-commits]

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

A few more minor nits to be cleared up, but otherwise LGTM. You should wait for 
@rsmith to sign off before committing in case he has further comments, however.




Comment at: lib/Sema/SemaExpr.cpp:4165
+bool Sema::TypeHasNoDeref(QualType Ty) {
+  // Strip off everything but attributes
+  QualType OldTy;

Missing a full stop at the end of the comment.



Comment at: lib/Sema/SemaExpr.cpp:4167
+  QualType OldTy;
+  while (Ty != OldTy && Ty->getTypeClass() != Type::Attributed) {
+OldTy = Ty;

I'd prefer to use `!isa(Ty)`; I think that's more clear than 
looking at the enumeration.



Comment at: test/Frontend/noderef.c:153
+
+  // Parenthesis
+  (((*((p); // expected-warning{{dereferencing p; was declared with a 
'noderef' type}}

Typo: Parentheses


Repository:
  rC Clang

https://reviews.llvm.org/D49511



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

[PATCH] D50994: Add a new flag and attributes to control static destructor registration

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Some minor nits that can be handled post-commit.




Comment at: lib/Sema/SemaDeclAttr.cpp:5921
+static void handleDestroyAttr(Sema &S, Decl *D, const ParsedAttr &A) {
+  if (!isa(D) || !cast(D)->hasGlobalStorage()) {
+S.Diag(D->getLocation(), diag::err_destroy_attr_on_non_static_var)

There is no need to check `VarDecl` here -- that's already done for you by the 
subject.



Comment at: lib/Sema/SemaDeclAttr.cpp:5927-5931
+  if (A.getKind() == ParsedAttr::AT_AlwaysDestroy) {
+handleSimpleAttributeWithExclusions(S, 
D, A);
+  } else {
+handleSimpleAttributeWithExclusions(S, 
D, A);
+  }

Elide braces.



Comment at: test/SemaCXX/no_destroy.cpp:33-34
+int main() {
+  [[clang::no_destroy]] int p; // expected-error{{no_destroy attribute can 
only be applied to a variable with static or thread storage duration}}
+  [[clang::always_destroy]] int p2; // expected-error{{always_destroy 
attribute can only be applied to a variable with static or thread storage 
duration}}
+  [[clang::no_destroy]] static int p3;

Please also add tests demonstrating that the attribute does not appertain to 
things that don't look like variables (like a function or struct declaration) 
and that it does not accept arguments.


Repository:
  rC Clang

https://reviews.llvm.org/D50994



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

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/fuchsia/FuchsiaTidyModule.cpp:41
+CheckFactories.registerCheck(
+"fuchsia-restrict-includes");
 CheckFactories.registerCheck(

I think this should be named `fuchsia-restrict-system-includes`.



Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:60
+StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  if (!llvm::is_contained(Check.getAllowedIncludes(), FileName) && IsAngled)
+// Bucket the allowed include directives by the id of the file they were

I'm not certain that `IsAngled` is sufficient. For instance, say the user 
prohibits use of `vector`, nothing prevents the user from writing `#include 
"vector"` to access the system header (assuming there is no user header with 
the same name). e.g., https://godbolt.org/g/Scfv3U

Perhaps we could use `SourceManager::isInSystemHeader()` or extract some logic 
from it to test whether the actual included file is in the system header search 
path?



Comment at: docs/ReleaseNotes.rst:116
+
+  Checks for allowed includes and suggests removal of any others. If no 
includes
+  are specified, the check will exit without issuing any warnings.

Be explicit that this only impacts system headers.



Comment at: docs/clang-tidy/checks/fuchsia-restrict-includes.rst:6-7
+
+Checks for allowed includes and suggests removal of any others. If no includes
+are specified, the check will exit without issuing any warnings. 
+

Should also be explicit about system headers.



Comment at: test/clang-tidy/Inputs/fuchsia-restrict-includes/system/r.h:2
+void f() {}
\ No newline at end of file


Please add in the EOF.



Comment at: 
test/clang-tidy/Inputs/fuchsia-restrict-includes/system/transitive.h:2
+#include 
\ No newline at end of file


Please add in the EOF.



Comment at: test/clang-tidy/fuchsia-restrict-includes.cpp:31
+}
\ No newline at end of file


Please add in the EOF.


https://reviews.llvm.org/D43778



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

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rjmccall.
aaron.ballman added a subscriber: rjmccall.
aaron.ballman added a comment.

In https://reviews.llvm.org/D42933#1091502, @jfb wrote:

> In https://reviews.llvm.org/D42933#1091234, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D42933#1090268, @jfb wrote:
> >
> > > I was just looking at this, and I think @arphaman's patch is pretty much 
> > > the right approach (with 2 suggested fixes below).
> > >
> > > I don't think the code we're currently warning on is broken: a user code 
> > > has `NSInteger` with `%zd` or `NSUInteger` with `%zu`, and on all 
> > > platforms which support those types the implementor has guaranteed that 
> > > `(sizeof(size_t) == sizeof(NSInteger)) && (sizeof(ssize_t) == 
> > > sizeof(NSUInteger))`.
> >
> >
> > Yes, but is this guaranteed to be the case or does it happen to be the 
> > case? I'm worried about the less mainstream uses where you might find ObjC 
> > code where this does not hold but -Wformat points out a portability concern.
>
>
> For Darwin platform, yes. That's why this diagnostic should only be squelched 
> for Darwin platforms.


Ah, I missed the fact that you were proposing this only for Darwin.

 I agree that, if we're playing C++ pedant and look at the typedefs, then 
 it's undefined behavior and the code is broken.
>>> 
>>> This is reason alone to diagnose the code, because the optimizer is welcome 
>>> to use that UB to perform transformations the programmer did not expect. 
>>> However, I'm not certain our optimizer is currently paying attention to 
>>> that UB directly, so this may not be an immediate issue (but could be a 
>>> pitfall for the future) but I'm not certain about other optimizers for 
>>> which portability would still be a concern.
>> 
>> I would honestly find it a bit surprising (and scary) if the optimizer 
>> actually took advantage of UB in the case where the size and alignment of 
>> the specifier and the actual type matches.
> 
> Hear, hear! I'm happy to fix any such optimization out of their misguided 
> optimism :-)

People used to think the same thing about many other optimizations that had 
non-local effects, but we've come to learn that these are not uncommon and 
incredibly hard for users to track down when it happens. Some enterprising soul 
could lower the call with an undef on the wrongly-typed argument and then who 
knows what happens. However, it's also reasonable for us to define undefined 
behavior for a given platform and that sounds like exactly this case.

If we're looking at only a specific change for Darwin, I think it's reasonable 
to make it to `-Wformat` rather than require `-Wformat-relaxed` (though we may 
still want to assert that the size and alignment of the underlying type match 
expectations).

I've added @rjmccall as a reviewer to see what his opinions are on this, but 
I'm inclined to say this is a reasonable change to make for Darwin targets.


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/fuchsia/RestrictSystemIncludesCheck.cpp:67
+  llvm::SmallDenseMap IncludeDirectives;
+  llvm::StringMap IsSystem;
+

Rather than use this `StringMap`, can you change `InclusionDirective()` to 
filter out includes you don't care about? Something along the lines of:
```
FileID FID = SM.translateFile(File);
assert(FID != FileID() && "Source Manager does not know about this header 
file");

bool Invalid;
const SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
if (!Invalid && SrcMgr::isSystem(Entry.getFile().getFileCharacteristic())) {
  // It's a system file.
}
```
This way you don't have to store all the path information and test against 
paths in `EndOfMainFile()`, unless I'm missing something. Note, this is 
untested code and perhaps that assert will fire (maybe SourceManager is unaware 
of this file by the time InclusionDirective() is called).

Alternatively, you could alter InclusionDirective() to also pass in the 
`CharacteristicKind` calculated by `Preprocessor::HandleIncludeDirective()` as 
that seems like generally useful information for the callback to have on hand.



Comment at: clang-tidy/fuchsia/RestrictSystemIncludesCheck.h:1-2
+//===--- RestrictSystemIncludesCheck.h - clang-tidy-*-
+//C++-*-===//
+//

This should only be on a single line --  you can remove some `-` characters.



Comment at: docs/clang-tidy/checks/fuchsia-restrict-system-includes.rst:32
+   A string containing a semi-colon separated list of allowed include 
filenames.
+   The default is an empty string, which allows all includes.

This default seems a bit odd to me, but perhaps it's fine. What's novel is that 
the check is a no-op by default, so how do Fuchsia developers get the correct 
list? Or is there no canonical list and developers are expected to populate 
their own manually?


https://reviews.llvm.org/D43778



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

[PATCH] D46300: [Clang] Implement function attribute no_stack_protector.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:1494
+def NoStackProtector : InheritableAttr {
+  let Spellings = [GCC<"no_stack_protector">];
+  let Subjects = SubjectList<[Function]>;

rnk wrote:
> aaron.ballman wrote:
> > manojgupta wrote:
> > > aaron.ballman wrote:
> > > > This is not a GCC attribute, so this should use the Clang spelling.
> > > > 
> > > > However, why spell the attribute this way rather than use the GCC 
> > > > spelling (`optimize("no-stack-protector")`?
> > > Thanks, I have changed it to use Clang spelling.
> > > 
> > > Regarding __attribute__((optimize("..."))), it is a generic facility in 
> > > GCC that works for many optimizer flags.
> > > Clang currently does not support this syntax currently instead preferring 
> > > its own version for some options e.g. -O0. 
> > > e.g.  
> > > ```
> > > __attribute__((optimize("O0")))  // clang version is 
> > > __attribute__((optnone)) 
> > > ```
> > > If we want to support the GCC syntax, future expectation would be support 
> > > more flags under this syntax. Is that the path we want to take (I do not 
> > > know the history related to previous syntax decisions but better GCC 
> > > compatibility will be a nice thing to have) 
> > The history of `optnone` predates my involvement with Clang and I've not 
> > been able to find the original review thread (I did find the one where I 
> > gave my LGTM on the original patch, but that was a resubmission after the 
> > original design was signed off).
> > 
> > I'm not keen on attributes that have the same semantics but differ in 
> > spelling from attributes supported by GCC unless there's a good reason to 
> > deviate. Given that we've already deviated, I'd like to understand why 
> > better -- I don't want to push you to implement something we've already 
> > decided was a poor design, but I also don't want to accept code if we can 
> > instead use syntax that is compatible with GCC.
> I do not think we want to pursue supporting generic optimization flags with 
> `__attribute__((optimize("...")))`.
> 
> Part of the reason we only support `optnone` is that LLVM's pass pipeline is 
> global to the entire module. It's not trivial to enable optimizations for a 
> single function, but it is much easier to have optimization passes skip 
> functions marked `optnone`.
Thank you, @rnk, that makes sense to me and seems like a solid reason for this 
approach.



Comment at: include/clang/Basic/AttrDocs.td:2746
+  let Content = [{
+Clang supports the ``__attribute__((no_stack_protector))`` attribute to disable
+stack protector on the specified functions. This attribute is useful for

to disable -> which disables the



Comment at: include/clang/Basic/AttrDocs.td:2747
+Clang supports the ``__attribute__((no_stack_protector))`` attribute to disable
+stack protector on the specified functions. This attribute is useful for
+selectively disabling stack protector on some functions when building with

functions -> function



Comment at: include/clang/Basic/AttrDocs.td:2748
+stack protector on the specified functions. This attribute is useful for
+selectively disabling stack protector on some functions when building with
+-fstack-protector compiler options.

disabling stack -> disabling the stack



Comment at: include/clang/Basic/AttrDocs.td:2749
+selectively disabling stack protector on some functions when building with
+-fstack-protector compiler options.
+

options -> option

Backticks around the compiler flag.



Comment at: include/clang/Basic/AttrDocs.td:2751
+
+For example, it disables stack protector for the function foo but function bar
+will still be built with stack protector with -fstack-protector option.

disables stack -> disables the stack

Please put backticks around `foo` and `bar` so they highlight as code.



Comment at: include/clang/Basic/AttrDocs.td:2752
+For example, it disables stack protector for the function foo but function bar
+will still be built with stack protector with -fstack-protector option.
+

with stack -> with the stack
with -fstack-protector -> with the -fstack-protector

Backticks around the compiler flag.



Comment at: include/clang/Basic/AttrDocs.td:2757
+int __attribute__((no_stack_protector))
+foo (int); // stack protection will be disabled for foo.
+

C requires parameters to be named; alternatively, you can use the C++ spelling 
of the attribute.



Comment at: include/clang/Basic/AttrDocs.td:2759
+
+int bar(int a); // bar can be built with stack protector.
+

with stack -> with the stack



Comment at: test/Sema/no_stack_protector.c:4
+void __attribute__((no_stack_protector)) foo() {}
+int __attribute__((no_stack_protector)

[PATCH] D46614: [clang] Adding CharacteristicKind to PPCallbacks::InclusionDirective

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM with a small nit.




Comment at: unittests/Lex/PPCallbacksTest.cpp:53
+this->Imported = Imported;
+this->FileType = FileType;
   }

Can you add a test that uses this field and checks its has the expected value?


https://reviews.llvm.org/D46614



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

[PATCH] D46614: [clang] Adding CharacteristicKind to PPCallbacks::InclusionDirective

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: unittests/Lex/PPCallbacksTest.cpp:142-143
   // the InclusionDirective callback.
-  CharSourceRange InclusionDirectiveFilenameRange(const char* SourceText, 
-  const char* HeaderPath, bool SystemHeader) {
+  InclusionDirectiveCallbacks *
+  InclusionDirectiveCallback(const char *SourceText,
+  const char *HeaderPath, bool SystemHeader) {

aaron.ballman wrote:
> The formatting looks off here, did clang-format do this?
It would probably be cleaner to provide the old interface as well, but define 
it to:
```
CharSourceRange InclusionDirectiveFilenameRange(const char* SourceText, const 
char* HeaderPath, bool SystemHeader) {
  return InclusionDirectiveCallback(SourceText, HeaderPath, 
SystemHeader)->FilenameRange;
}
```



Comment at: unittests/Lex/PPCallbacksTest.cpp:142-144
+  InclusionDirectiveCallbacks *
+  InclusionDirectiveCallback(const char *SourceText,
+  const char *HeaderPath, bool SystemHeader) {

The formatting looks off here, did clang-format do this?


https://reviews.llvm.org/D46614



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

[PATCH] D46614: [clang] Adding CharacteristicKind to PPCallbacks::InclusionDirective

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


https://reviews.llvm.org/D46614



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

[PATCH] D46615: [tools] Updating PPCallbacks::InclusionDirective calls

[Aaron Ballman via Phabricator via cfe-commits]

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

Aside from a minor nit, LGTM (no need for more review, you can fix the nit and 
commit).




Comment at: clang-move/ClangMove.cpp:135
+  const clang::Module * /*Imported*/,
+  SrcMgr::CharacteristicKind FileType) override {
 if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc)))

Put `FileType` into comments so the parameter is unnamed like the other unused 
ones.


https://reviews.llvm.org/D46615



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

[PATCH] D46300: [Clang] Implement function attribute no_stack_protector.

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D46300



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

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: docs/clang-tidy/checks/fuchsia-restrict-system-includes.rst:32
+   A string containing a semi-colon separated list of allowed include 
filenames.
+   The default is an empty string, which allows all includes.

juliehockett wrote:
> aaron.ballman wrote:
> > This default seems a bit odd to me, but perhaps it's fine. What's novel is 
> > that the check is a no-op by default, so how do Fuchsia developers get the 
> > correct list? Or is there no canonical list and developers are expected to 
> > populate their own manually?
> The idea is that it's a case-by-case basis -- this may change at some point 
> in the future if we decide there's a standard whitelist of system includes 
> across the board, but at the moment the thought is to allow everything in 
> some places, and use this check to limit them in others. It'll need to be 
> populated on a case-by-case basis, since different directories will have 
> different requirements.
So there's never a case where you need to prohibit use of all system includes? 
Right now, an empty string means "allow all includes" while a non-empty string 
means "allow only these includes", so there's no way to prohibit all includes 
except by listing them manually. I'm wondering whether you want to use a glob 
for allowed file names, which gives you extra flexibility. e.g., `*` allows all 
includes (default value), `foo.h` allows only , `experimental/*` allows 
all headers in the experimental directory, `cstd*` allows  while 
prohibiting , and the empty string will disallow all system headers.

Perhaps you don't need that level of flexibility, but for the use cases I'm 
imagining it seems more user friendly to be able to write `cstd*` rather than 
manually listing out all the C++ headers explicitly.


https://reviews.llvm.org/D43778



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

[PATCH] D46614: [clang] Adding CharacteristicKind to PPCallbacks::InclusionDirective

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: unittests/Lex/PPCallbacksTest.cpp:140
 
+  std::unique_ptr getPreprocessor(const char *SourceText,
+const char *HeaderPath,

This function appears to be unused?



Comment at: unittests/Lex/PPCallbacksTest.cpp:179
 
-Preprocessor PP(std::make_shared(), Diags, LangOpts,
-SourceMgr, PCMCache, HeaderInfo, ModLoader,
-/*IILookup =*/nullptr,
-/*OwnsHeaderSearch =*/false);
-PP.Initialize(*Target);
-InclusionDirectiveCallbacks* Callbacks = new InclusionDirectiveCallbacks;
-PP.addPPCallbacks(std::unique_ptr(Callbacks));
+std::unique_ptr PP = llvm::make_unique(
+std::make_shared(), Diags, LangOpts, SourceMgr,

Did you intend to make use of it here?



Comment at: unittests/Lex/PPCallbacksTest.cpp:200
+
+std::unique_ptr PP = llvm::make_unique(
+std::make_shared(), Diags, LangOpts, SourceMgr,

and here?


https://reviews.llvm.org/D46614



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

[PATCH] D46614: [clang] Adding CharacteristicKind to PPCallbacks::InclusionDirective

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: unittests/Lex/PPCallbacksTest.cpp:155-160
+std::unique_ptr PP = llvm::make_unique(
+std::make_shared(), Diags, LangOpts, SourceMgr,
+PCMCache, HeaderInfo, ModLoader,
+/*IILookup =*/nullptr,
+/*OwnsHeaderSearch =*/false);
+return InclusionDirectiveCallback(PP.get())->FilenameRange;

I'm likely just missing something, but why is the `unique_ptr` required at all? 
The `Preprocessor` object will be destroyed on exit from this function, so it 
seems like it could be an automatic variable that's passed by reference to 
`InclusionDirectiveCallback()`, and same below.


https://reviews.llvm.org/D46614



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

[PATCH] D46614: [clang] Adding CharacteristicKind to PPCallbacks::InclusionDirective

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


https://reviews.llvm.org/D46614



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

[PATCH] D45835: Add new driver mode for dumping compiler options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ping.


https://reviews.llvm.org/D45835



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

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

[Aaron Ballman via Phabricator via cfe-commits]

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

Thank you for adding the glob feature, I think that will be very powerful in 
practice. Aside from a minor nit, this LGTM!




Comment at: clang-tidy/fuchsia/RestrictSystemIncludesCheck.h:35
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  bool contains(StringRef FileName) {
+return AllowedIncludesGlobList.contains(FileName);

Function can be marked `const`.


https://reviews.llvm.org/D43778



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

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Expounding on my concerns a bit -- I'm worried about all the other places 
calling `isUndeducedType()` and whether they're also at risk and thus a better 
fix is to change `isUndeducedType()` to pay attention to the language mode.




Comment at: lib/Sema/SemaDecl.cpp:12611
+// false on C++14's auto return type without trailing return type.
+DeducedType *DT = FD->getReturnType()->getContainedDeducedType();
+if (DT && DT->getDeducedType().isNull())

This can be marked `const`.


Repository:
  rC Clang

https://reviews.llvm.org/D44480



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

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/fuchsia/RestrictSystemIncludesCheck.h:35
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  bool contains(StringRef FileName) {
+return AllowedIncludesGlobList.contains(FileName);

juliehockett wrote:
> aaron.ballman wrote:
> > Function can be marked `const`.
> GlobList::contains isn't `const`, so it can't...
Bah, I didn't see that. You can ignore, sorry for the noise.


https://reviews.llvm.org/D43778



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

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 146472.
aaron.ballman added a comment.

Addresses review comments.


https://reviews.llvm.org/D46112

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaType.cpp
  test/CodeGen/c11atomics.c
  test/Sema/atomic-type.c
  test/Sema/atomic-type.cpp

Index: test/Sema/atomic-type.cpp
===
--- test/Sema/atomic-type.cpp
+++ test/Sema/atomic-type.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+struct S;
+
+void f(_Atomic S *s);
+
+struct S { int a, b; };
+
+void f(_Atomic S *s) {
+  S s2;
+  (void)__atomic_load(s, &s2, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(S) *' invalid)}}
+  (void)__c11_atomic_load(s, 5);
+}
+
Index: test/Sema/atomic-type.c
===
--- test/Sema/atomic-type.c
+++ test/Sema/atomic-type.c
@@ -16,7 +16,17 @@
 extern _Atomic(int (*)(int(*)[10], int(*)[10])) mergetest;
 
 _Atomic(int()) error1; // expected-error {{_Atomic cannot be applied to function type}}
-_Atomic(struct ErrorS) error2; // expected-error {{_Atomic cannot be applied to incomplete type}} expected-note {{forward declaration}}
+_Atomic(struct ErrorS) error2;
 _Atomic(int[10]) error3; // expected-error {{_Atomic cannot be applied to array type}}
 _Atomic(const int) error4; // expected-error {{_Atomic cannot be applied to qualified type}}
 _Atomic(_Atomic(int)) error5; // expected-error {{_Atomic cannot be applied to atomic type}}
+
+void g(_Atomic void *ptr);
+void h(_Atomic struct Incomplete *ptr); // expected-warning {{declaration of 'struct Incomplete' will not be visible outside of this function}}
+
+void test_atomic_void_ptr(_Atomic void *addr) {
+  int i;
+  (void)__atomic_load(addr, &i, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(void) *' invalid)}}
+  (void)__c11_atomic_load(addr, 5); // expected-error {{invalid use of incomplete type 'void'}}
+  _Static_assert(__atomic_is_lock_free(1, addr), "");
+}
Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -61,7 +61,7 @@
   // we have to generate an atomic add, which returns the old value, and then a
   // non-atomic add.
   // CHECK: atomicrmw add i32* @i, i32 1 seq_cst
-  // CHECK: add i32 
+  // CHECK: add i32
   ++i;
   // CHECK: atomicrmw add i64* @l, i64 1 seq_cst
   // CHECK: add i64
@@ -475,6 +475,25 @@
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
 
+int test_atomic_void_ptr(_Atomic void *addr) {
+  // CHECK-LABEL: @test_atomic_void_ptr(
+  // CHECK:   [[ADDR_ARG:%.*]] = alloca { i8, [1 x i8] }*, align 4
+  // CHECK:   [[ATOMIC_RES:%.*]] = alloca i32, align 4
+  // CHECK:   store { i8, [1 x i8] }* %addr, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE_VP:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR_LOCK_FREE_VP]] to i8*
+  // CHECK:   call arm_aapcscc i32 @__atomic_is_lock_free(i32 1, i8* [[ADDR_LOCK_FREE]])
+  // CHECK:   [[ADDR:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[CAST1:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR]] to i32*
+  // CHECK:   [[CAST2:%.*]] = bitcast i32* [[CAST1]] to i8*
+  // CHECK:   [[CALL:%.*]] = call arm_aapcscc i32 @__atomic_load_4(i8* [[CAST2]], i32 5)
+  // CHECK:   store i32 [[CALL]], i32* [[ATOMIC_RES]], align 4
+  // CHECK:   [[RES:%.*]] = load i32, i32* [[ATOMIC_RES]], align 4
+  // CHECK:   ret i32 [[RES]]
+  (void)__atomic_is_lock_free(1, addr);
+  return (int)__c11_atomic_load((_Atomic int *)addr, 5);
+}
+
 struct Empty {};
 
 struct Empty test_empty_struct_load(_Atomic(struct Empty)* empty) {
@@ -488,3 +507,25 @@
   // CHECK: call arm_aapcscc void @__atomic_store_1(i8* %{{.*}}, i8 zeroext %{{.*}}, i32 5)
   __c11_atomic_store(empty, value, 5);
 }
+
+typedef struct T T;
+void test_struct_copy(_Atomic(T) *t1, _Atomic(T) *t2);
+struct T { int a, b; };
+void test_struct_copy(_Atomic(T) *t1, _Atomic(T) *t2) {
+  // CHECK-LABEL: @test_struct_copy(
+  // CHECK:  [[T1:%.*]] = alloca %struct.T*, align 4
+  // CHECK:  [[T2:%.*]] = alloca %struct.T*, align 4
+  // CHECK:  [[TEMP:%.*]] = alloca %struct.T, align 8
+  // CHECK:  store %struct.T* %t1, %struct.T** [[T1]], align 4
+  // CHECK:  store %struct.T* %t2, %struct.T** [[T2]], align 4
+  // CHECK:  [[LOAD1:%.*]] = load %struct.T*, %struct.T** [[T1]], align 4
+  // CHECK:  [[LOAD2:%.*]] = load %struct.T*, %struct.T** [[T2]], align 4
+  // CHECK:  [[CAST1:%.*]] = bitcast %struct.T* [[LOAD2]] to i8*
+  // CHECK:  [[CAST2:%.*]] = bitcast %struct.T* [[TEMP]] to i8*
+  // CHECK:  call arm_aapcscc void @__atomic_load(i3

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 146474.
aaron.ballman added a comment.

Modifications based on review feedback.


https://reviews.llvm.org/D46112

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/Type.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaType.cpp
  test/CodeGen/c11atomics.c
  test/Sema/atomic-type.c
  test/SemaCXX/atomic-type.cpp

Index: test/SemaCXX/atomic-type.cpp
===
--- test/SemaCXX/atomic-type.cpp
+++ test/SemaCXX/atomic-type.cpp
@@ -2,17 +2,17 @@
 // RUN: %clang_cc1 -verify -pedantic %s -std=c++11
 
 template struct atomic {
-  _Atomic(T) value;
+  _Atomic(T) value; // expected-error {{field has incomplete type '_Atomic(user::inner)'}}
 
   void f() _Atomic; // expected-error {{expected ';' at end of declaration list}}
 };
 
 template struct user {
   struct inner { char n[sizeof(T)]; };
-  atomic i;
+  atomic i; // expected-note {{in instantiation}}
 };
 
-user u;
+user u; // expected-note {{in instantiation}}
 
 // Test overloading behavior of atomics.
 struct A { };
@@ -94,3 +94,15 @@
 bool PR21836(_Atomic(int) *x) {
 return *x;
 }
+
+struct T;
+
+void f(_Atomic T *t);
+
+struct T { int a, b; };
+
+void f(_Atomic T *t) {
+  T t2;
+  (void)__atomic_load(t, &t2, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(T) *' invalid)}}
+  (void)__c11_atomic_load(t, 5);
+}
Index: test/Sema/atomic-type.c
===
--- test/Sema/atomic-type.c
+++ test/Sema/atomic-type.c
@@ -16,7 +16,22 @@
 extern _Atomic(int (*)(int(*)[10], int(*)[10])) mergetest;
 
 _Atomic(int()) error1; // expected-error {{_Atomic cannot be applied to function type}}
-_Atomic(struct ErrorS) error2; // expected-error {{_Atomic cannot be applied to incomplete type}} expected-note {{forward declaration}}
+_Atomic(struct ErrorS) error2; // expected-error {{tentative definition has type '_Atomic(struct ErrorS)' that is never completed}}
 _Atomic(int[10]) error3; // expected-error {{_Atomic cannot be applied to array type}}
 _Atomic(const int) error4; // expected-error {{_Atomic cannot be applied to qualified type}}
 _Atomic(_Atomic(int)) error5; // expected-error {{_Atomic cannot be applied to atomic type}}
+
+void g(_Atomic void *ptr);
+void h(_Atomic struct Incomplete *ptr); // expected-warning {{declaration of 'struct Incomplete' will not be visible outside of this function}}
+
+void test_atomic_void_ptr(_Atomic void *addr) {
+  int i;
+  (void)__atomic_load(addr, &i, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(void) *' invalid)}}
+  (void)__c11_atomic_load(addr, 5); // expected-error {{invalid use of incomplete type 'void'}}
+  _Static_assert(__atomic_is_lock_free(1, addr), "");
+}
+
+typedef struct S S;
+void test_atomic_incomplete_struct_assignment(_Atomic S *s, _Atomic S *s2) {
+  *s = *s2; // expected-error {{incomplete type '_Atomic(S)' is not assignable}}
+}
Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -61,7 +61,7 @@
   // we have to generate an atomic add, which returns the old value, and then a
   // non-atomic add.
   // CHECK: atomicrmw add i32* @i, i32 1 seq_cst
-  // CHECK: add i32 
+  // CHECK: add i32
   ++i;
   // CHECK: atomicrmw add i64* @l, i64 1 seq_cst
   // CHECK: add i64
@@ -475,6 +475,25 @@
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
 
+int test_atomic_void_ptr(_Atomic void *addr) {
+  // CHECK-LABEL: @test_atomic_void_ptr(
+  // CHECK:   [[ADDR_ARG:%.*]] = alloca { i8, [1 x i8] }*, align 4
+  // CHECK:   [[ATOMIC_RES:%.*]] = alloca i32, align 4
+  // CHECK:   store { i8, [1 x i8] }* %addr, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE_VP:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR_LOCK_FREE_VP]] to i8*
+  // CHECK:   call arm_aapcscc i32 @__atomic_is_lock_free(i32 1, i8* [[ADDR_LOCK_FREE]])
+  // CHECK:   [[ADDR:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[CAST1:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR]] to i32*
+  // CHECK:   [[CAST2:%.*]] = bitcast i32* [[CAST1]] to i8*
+  // CHECK:   [[CALL:%.*]] = call arm_aapcscc i32 @__atomic_load_4(i8* [[CAST2]], i32 5)
+  // CHECK:   store i32 [[CALL]], i32* [[ATOMIC_RES]], align 4
+  // CHECK:   [[RES:%.*]] = load i32, i32* [[ATOMIC_RES]], align 4
+  // CHECK:   ret i32 [[RES]]
+  (void)__atomic_is_lock_free(1, addr);
+  return (int)__c11_atomic_load((_Atomic int *)addr, 5);
+}
+
 struct Empty {};
 
 struct Empty test_empty_struct_load(_Atomic(struct Empty)* empty) {
@@ -488,3 +507,25 @@
   // CHECK: call arm_aapcscc void @__atomic_store_1(i8* %{{.*}}, i8 ze

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D46112#1091981, @efriedma wrote:

> I think the request was that we check that a type is trivially copyable when 
> we perform an atomic operation?  I don't see the code for that anywhere.


Sorry about that -- I didn't notice that GNU was handled while C11 was not. 
That's been updated now.

> Also needs some test coverage for atomic operations which aren't calls, like 
> "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".

Thank you for pointing this out -- that uncovered an issue where we were not 
properly diagnosing the types as being incomplete. I've added a new test case 
and rolled the contents of Sema\atomic-type.cpp (which I added in an earlier 
patch) into SemaCXX\atomic-type.cpp (which already existed and I missed it).

I believe the change I made to `Type::isIncompleteType()` is correct, but was 
surprised by the new behavior in SemaCXX/atomic-type.cpp that resulted. It 
appears that the `RecordDecl` for "struct inner" has `IsCompleteDefinition` set 
to `false` while instantiating `struct atomic`, but I'm not familiar enough 
with the template instantiation process to know whether this is reasonable or 
not.


https://reviews.llvm.org/D46112



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

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 146524.
aaron.ballman added a comment.

Look through atomic types when checking type completeness during template 
instantiation.


https://reviews.llvm.org/D46112

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/Type.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaType.cpp
  test/CodeGen/c11atomics.c
  test/Sema/atomic-type.c
  test/SemaCXX/atomic-type.cpp

Index: test/SemaCXX/atomic-type.cpp
===
--- test/SemaCXX/atomic-type.cpp
+++ test/SemaCXX/atomic-type.cpp
@@ -94,3 +94,15 @@
 bool PR21836(_Atomic(int) *x) {
 return *x;
 }
+
+struct T;
+
+void f(_Atomic T *t);
+
+struct T { int a, b; };
+
+void f(_Atomic T *t) {
+  T t2;
+  (void)__atomic_load(t, &t2, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(T) *' invalid)}}
+  (void)__c11_atomic_load(t, 5);
+}
Index: test/Sema/atomic-type.c
===
--- test/Sema/atomic-type.c
+++ test/Sema/atomic-type.c
@@ -16,7 +16,22 @@
 extern _Atomic(int (*)(int(*)[10], int(*)[10])) mergetest;
 
 _Atomic(int()) error1; // expected-error {{_Atomic cannot be applied to function type}}
-_Atomic(struct ErrorS) error2; // expected-error {{_Atomic cannot be applied to incomplete type}} expected-note {{forward declaration}}
+_Atomic(struct ErrorS) error2; // expected-error {{tentative definition has type '_Atomic(struct ErrorS)' that is never completed}}
 _Atomic(int[10]) error3; // expected-error {{_Atomic cannot be applied to array type}}
 _Atomic(const int) error4; // expected-error {{_Atomic cannot be applied to qualified type}}
 _Atomic(_Atomic(int)) error5; // expected-error {{_Atomic cannot be applied to atomic type}}
+
+void g(_Atomic void *ptr);
+void h(_Atomic struct Incomplete *ptr); // expected-warning {{declaration of 'struct Incomplete' will not be visible outside of this function}}
+
+void test_atomic_void_ptr(_Atomic void *addr) {
+  int i;
+  (void)__atomic_load(addr, &i, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(void) *' invalid)}}
+  (void)__c11_atomic_load(addr, 5); // expected-error {{invalid use of incomplete type 'void'}}
+  _Static_assert(__atomic_is_lock_free(1, addr), "");
+}
+
+typedef struct S S;
+void test_atomic_incomplete_struct_assignment(_Atomic S *s, _Atomic S *s2) {
+  *s = *s2; // expected-error {{incomplete type '_Atomic(S)' is not assignable}}
+}
Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -61,7 +61,7 @@
   // we have to generate an atomic add, which returns the old value, and then a
   // non-atomic add.
   // CHECK: atomicrmw add i32* @i, i32 1 seq_cst
-  // CHECK: add i32 
+  // CHECK: add i32
   ++i;
   // CHECK: atomicrmw add i64* @l, i64 1 seq_cst
   // CHECK: add i64
@@ -475,6 +475,25 @@
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
 
+int test_atomic_void_ptr(_Atomic void *addr) {
+  // CHECK-LABEL: @test_atomic_void_ptr(
+  // CHECK:   [[ADDR_ARG:%.*]] = alloca { i8, [1 x i8] }*, align 4
+  // CHECK:   [[ATOMIC_RES:%.*]] = alloca i32, align 4
+  // CHECK:   store { i8, [1 x i8] }* %addr, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE_VP:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR_LOCK_FREE_VP]] to i8*
+  // CHECK:   call arm_aapcscc i32 @__atomic_is_lock_free(i32 1, i8* [[ADDR_LOCK_FREE]])
+  // CHECK:   [[ADDR:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[CAST1:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR]] to i32*
+  // CHECK:   [[CAST2:%.*]] = bitcast i32* [[CAST1]] to i8*
+  // CHECK:   [[CALL:%.*]] = call arm_aapcscc i32 @__atomic_load_4(i8* [[CAST2]], i32 5)
+  // CHECK:   store i32 [[CALL]], i32* [[ATOMIC_RES]], align 4
+  // CHECK:   [[RES:%.*]] = load i32, i32* [[ATOMIC_RES]], align 4
+  // CHECK:   ret i32 [[RES]]
+  (void)__atomic_is_lock_free(1, addr);
+  return (int)__c11_atomic_load((_Atomic int *)addr, 5);
+}
+
 struct Empty {};
 
 struct Empty test_empty_struct_load(_Atomic(struct Empty)* empty) {
@@ -488,3 +507,25 @@
   // CHECK: call arm_aapcscc void @__atomic_store_1(i8* %{{.*}}, i8 zeroext %{{.*}}, i32 5)
   __c11_atomic_store(empty, value, 5);
 }
+
+typedef struct T T;
+void test_struct_copy(_Atomic(T) *t1, _Atomic(T) *t2);
+struct T { int a, b; };
+void test_struct_copy(_Atomic(T) *t1, _Atomic(T) *t2) {
+  // CHECK-LABEL: @test_struct_copy(
+  // CHECK:  [[T1:%.*]] = alloca %struct.T*, align 4
+  // CHECK:  [[T2:%.*]] = alloca %struct.T*, align 4
+  // CHECK:  [[TEMP:%.*]] = alloca %struct.T, align 8
+  // CHECK:  store %struct.T* %t1, %struct.T** [[T1]], align 4
+  // CHEC

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: test/SemaCXX/atomic-type.cpp:5
 template struct atomic {
-  _Atomic(T) value;
+  _Atomic(T) value; // expected-error {{field has incomplete type 
'_Atomic(user::inner)'}}
 

rsmith wrote:
> This is a bug. You need to teach `RequireCompleteType` to look through atomic 
> types when looking for a class type to instantiate.
Thank you for the explanation -- I think I've addressed this in the latest 
patch.


https://reviews.llvm.org/D46112



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

[PATCH] D45835: Add new driver mode for dumping compiler options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 146529.
aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.

Updated based on review feedback.


https://reviews.llvm.org/D45835

Files:
  include/clang/Basic/Features.def
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/FrontendActions.h
  include/clang/Frontend/FrontendOptions.h
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/Lex/PPMacroExpansion.cpp
  test/Frontend/compiler-options-dump.cpp

Index: test/Frontend/compiler-options-dump.cpp
===
--- test/Frontend/compiler-options-dump.cpp
+++ test/Frontend/compiler-options-dump.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -compiler-options-dump -std=c++03 %s -o - | FileCheck %s --check-prefix=CXX03
+// RUN: %clang_cc1 -compiler-options-dump -std=c++17 %s -o - | FileCheck %s --check-prefix=CXX17
+// RUN: %clang_cc1 -compiler-options-dump -std=c99 -ffast-math -x c %s -o - | FileCheck %s --check-prefix=C99
+
+// CXX03: "features"
+// CXX03: "cxx_auto_type" : false
+// CXX03: "cxx_range_for" : false
+// CXX03: "extensions"
+// CXX03: "cxx_range_for" : true
+
+// CXX17: "features"
+// CXX17: "cxx_auto_type" : true
+// CXX17: "cxx_range_for" : true
+// CXX17: "extensions"
+// CXX17: "cxx_range_for" : true
+
+// C99: "features"
+// C99: "c_alignas" : false
+// C99: "c_atomic" : false
+// C99: "cxx_auto_type" : false
+// C99: "cxx_range_for" : false
+// C99: "extensions"
+// C99: "c_alignas" : true
+// C99: "c_atomic" : true
+// C99: "cxx_range_for" : false
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -1099,187 +1099,11 @@
   if (Feature.startswith("__") && Feature.endswith("__") && Feature.size() >= 4)
 Feature = Feature.substr(2, Feature.size() - 4);
 
+#define FEATURE(Name, Predicate) .Case(#Name, Predicate)
   return llvm::StringSwitch(Feature)
-  .Case("address_sanitizer",
-LangOpts.Sanitize.hasOneOf(SanitizerKind::Address |
-   SanitizerKind::KernelAddress))
-  .Case("hwaddress_sanitizer",
-LangOpts.Sanitize.hasOneOf(SanitizerKind::HWAddress |
-   SanitizerKind::KernelHWAddress))
-  .Case("assume_nonnull", true)
-  .Case("attribute_analyzer_noreturn", true)
-  .Case("attribute_availability", true)
-  .Case("attribute_availability_with_message", true)
-  .Case("attribute_availability_app_extension", true)
-  .Case("attribute_availability_with_version_underscores", true)
-  .Case("attribute_availability_tvos", true)
-  .Case("attribute_availability_watchos", true)
-  .Case("attribute_availability_with_strict", true)
-  .Case("attribute_availability_with_replacement", true)
-  .Case("attribute_availability_in_templates", true)
-  .Case("attribute_cf_returns_not_retained", true)
-  .Case("attribute_cf_returns_retained", true)
-  .Case("attribute_cf_returns_on_parameters", true)
-  .Case("attribute_deprecated_with_message", true)
-  .Case("attribute_deprecated_with_replacement", true)
-  .Case("attribute_ext_vector_type", true)
-  .Case("attribute_ns_returns_not_retained", true)
-  .Case("attribute_ns_returns_retained", true)
-  .Case("attribute_ns_consumes_self", true)
-  .Case("attribute_ns_consumed", true)
-  .Case("attribute_cf_consumed", true)
-  .Case("attribute_objc_ivar_unused", true)
-  .Case("attribute_objc_method_family", true)
-  .Case("attribute_overloadable", true)
-  .Case("attribute_unavailable_with_message", true)
-  .Case("attribute_unused_on_fields", true)
-  .Case("attribute_diagnose_if_objc", true)
-  .Case("blocks", LangOpts.Blocks)
-  .Case("c_thread_safety_attributes", true)
-  .Case("cxx_exceptions", LangOpts.CXXExceptions)
-  .Case("cxx_rtti", LangOpts.RTTI && LangOpts.RTTIData)
-  .Case("enumerator_attributes", true)
-  .Case("nullability", true)
-  .Case("nullability_on_arrays", true)
-  .Case("memory_sanitizer", LangOpts.Sanitize.has(SanitizerKind::Memory))
-  .Case("thread_sanitizer", LangOpts.Sanitize.has(SanitizerKind::Thread))
-  .Case("dataflow_sanitizer",
-LangOpts.Sanitize.has(SanitizerKind::DataFlow))
-  .Case("efficiency_sanitizer",
-LangOpts.Sanitize.hasOneOf(SanitizerKind::Efficiency))
-  .Case("scudo", LangOpts.Sanitize.hasOneOf(SanitizerKind::Scudo))
-  // Objective-C features
-  .Case("objc_arr", LangOpts.ObjCAutoRefCount) // FIXME: REMOVE?
-  .Case("objc_arc", LangOpts.ObjCAutoRefCount)
-  .Case("objc_arc_fields", true)
-  .Case("objc_arc_weak", LangOpts.ObjCWeak)
-  .Case("objc_default_synthesize_properties", LangOpts.ObjC2)
-  .Case("objc_fixed_enum", LangOpts

[PATCH] D45835: Add new driver mode for dumping compiler options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Frontend/FrontendActions.cpp:779
+  {
+std::string Str;
+#define FEATURE(Name, Predicate)   
\

lebedev.ri wrote:
> This is likely to do an allocation for each feature.
> Maybe consider `llvm::SmallString<64>`
64 bytes seemed a bit tight, so I went with 128 instead. Likely still a bit too 
small, but shouldn't be too bad.


https://reviews.llvm.org/D45835



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

[PATCH] D45702: [clang-tidy] Add a new check, readability-simplify-subscript-expr, that simplifies subscript expressions.

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM with a few minor nits to be fixed.




Comment at: clang-tidy/readability/SimplifySubscriptExprCheck.cpp:53-54
+  const auto *Call = Result.Nodes.getNodeAs("call");
+  if (Result.Context->getSourceManager().isMacroBodyExpansion(
+  Call->getExprLoc())) {
+return;

Can remove the braces.



Comment at: clang-tidy/readability/SimplifySubscriptExprCheck.cpp:62
+   "'data()'; did you mean to use 'operator[]'?");
+  if (Member->isArrow()) {
+DiagBuilder << FixItHint::CreateInsertion(Member->getLocStart(), "(*")

Remove braces.



Comment at: clang-tidy/readability/SimplifySubscriptExprCheck.h:22
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-data-call.html
+class SimplifySubscriptExprCheck : public ClangTidyCheck {

This URL looks stale.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45702



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

[PATCH] D45093: [AST] Fix -ast-print for _Bool when have diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D45093



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

[PATCH] D46639: [CodeComplete] Provide completion in decls even for incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D46639



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

[PATCH] D46805: If some platforms do not support an attribute, we should exclude the platform

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this patch -- be sure to add tests that ensure the 
attribute is properly prohibited on the expected targets.




Comment at: include/clang/Basic/Attr.td:299-300
   list ObjectFormats;
+  // It indicates that a certain attribute is not supported on a specific
+  // platform, turn on support for this attribute by default
+  bit Negated = 0;

How about: `If set to false, indicates the attribute is supported only on the 
given target. If set to true, indicates the attribute is supported on all 
targets except the given target.`



Comment at: include/clang/Basic/Attr.td:324-325
 
+// We do not support the alias attribute on Apple platforms,
+// so I define a platform other than the Apple platform.
+def TargetDarwinNegative : TargetArch<["x86", "x86_64"]> {

This comment is a bit too specific, I'd probably drop the comment entirely as 
the code is sufficiently clear.



Comment at: include/clang/Basic/Attr.td:326
+// so I define a platform other than the Apple platform.
+def TargetDarwinNegative : TargetArch<["x86", "x86_64"]> {
+  let OSes = ["MacOSX"];

I think we'll want to pattern the names like `TargetNotFoo`.

One other thing is: this isn't really Darwin -- for instance, there's ARM and 
AArch64 variants, not just x86-based targets.



Comment at: include/clang/Basic/Attr.td:327
+def TargetDarwinNegative : TargetArch<["x86", "x86_64"]> {
+  let OSes = ["MacOSX"];
+  let Negated = 1;

I would expect the OS would be named "Darwin" and not "MacOSX" given the name 
of the target. Which flavor should be used, or should both be used?



Comment at: utils/TableGen/ClangAttrEmitter.cpp:2789
+  // is not supported on this platform
+  if(R->getValueAsBit("Negated"))
+Test = "!(" + Test + ")";

The formatting is off here -- there should be a space after the `if`. I would 
recommend running your patch through clang-format 
(https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).


Repository:
  rC Clang

https://reviews.llvm.org/D46805



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

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 146594.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Updated based on review comments.


https://reviews.llvm.org/D46112

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/Type.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaType.cpp
  test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
  test/CodeGen/c11atomics.c
  test/Sema/atomic-type.c
  test/SemaCXX/atomic-type.cpp
  test/SemaObjC/arc.m

Index: test/SemaObjC/arc.m
===
--- test/SemaObjC/arc.m
+++ test/SemaObjC/arc.m
@@ -410,7 +410,7 @@
   [v test16_6: 0];
 }
 
-@class Test17; // expected-note 2{{forward declaration of class here}}
+@class Test17; // expected-note 3{{forward declaration of class here}}
 @protocol Test17p
 - (void) test17;
 + (void) test17;
Index: test/SemaCXX/atomic-type.cpp
===
--- test/SemaCXX/atomic-type.cpp
+++ test/SemaCXX/atomic-type.cpp
@@ -94,3 +94,15 @@
 bool PR21836(_Atomic(int) *x) {
 return *x;
 }
+
+struct T;
+
+void f(_Atomic T *t);
+
+struct T { int a, b; };
+
+void f(_Atomic T *t) {
+  T t2;
+  (void)__atomic_load(t, &t2, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(T) *' invalid)}}
+  (void)__c11_atomic_load(t, 5);
+}
Index: test/Sema/atomic-type.c
===
--- test/Sema/atomic-type.c
+++ test/Sema/atomic-type.c
@@ -16,7 +16,23 @@
 extern _Atomic(int (*)(int(*)[10], int(*)[10])) mergetest;
 
 _Atomic(int()) error1; // expected-error {{_Atomic cannot be applied to function type}}
-_Atomic(struct ErrorS) error2; // expected-error {{_Atomic cannot be applied to incomplete type}} expected-note {{forward declaration}}
+_Atomic(struct ErrorS) error2; // expected-error {{tentative definition has type '_Atomic(struct ErrorS)' that is never completed}} \
+   // expected-note {{forward declaration of 'struct ErrorS'}}
 _Atomic(int[10]) error3; // expected-error {{_Atomic cannot be applied to array type}}
 _Atomic(const int) error4; // expected-error {{_Atomic cannot be applied to qualified type}}
 _Atomic(_Atomic(int)) error5; // expected-error {{_Atomic cannot be applied to atomic type}}
+
+void g(_Atomic void *ptr);
+void h(_Atomic struct Incomplete *ptr); // expected-warning {{declaration of 'struct Incomplete' will not be visible outside of this function}}
+
+void test_atomic_void_ptr(_Atomic void *addr) {
+  int i;
+  (void)__atomic_load(addr, &i, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(void) *' invalid)}}
+  (void)__c11_atomic_load(addr, 5); // expected-error {{invalid use of incomplete type 'void'}}
+  _Static_assert(__atomic_is_lock_free(1, addr), "");
+}
+
+typedef struct S S; // expected-note {{forward declaration of 'struct S'}}
+void test_atomic_incomplete_struct_assignment(_Atomic S *s, _Atomic S *s2) {
+  *s = *s2; // expected-error {{incomplete type '_Atomic(S)' is not assignable}}
+}
Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -61,7 +61,7 @@
   // we have to generate an atomic add, which returns the old value, and then a
   // non-atomic add.
   // CHECK: atomicrmw add i32* @i, i32 1 seq_cst
-  // CHECK: add i32 
+  // CHECK: add i32
   ++i;
   // CHECK: atomicrmw add i64* @l, i64 1 seq_cst
   // CHECK: add i64
@@ -475,6 +475,25 @@
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
 
+int test_atomic_void_ptr(_Atomic void *addr) {
+  // CHECK-LABEL: @test_atomic_void_ptr(
+  // CHECK:   [[ADDR_ARG:%.*]] = alloca { i8, [1 x i8] }*, align 4
+  // CHECK:   [[ATOMIC_RES:%.*]] = alloca i32, align 4
+  // CHECK:   store { i8, [1 x i8] }* %addr, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE_VP:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR_LOCK_FREE_VP]] to i8*
+  // CHECK:   call arm_aapcscc i32 @__atomic_is_lock_free(i32 1, i8* [[ADDR_LOCK_FREE]])
+  // CHECK:   [[ADDR:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[CAST1:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR]] to i32*
+  // CHECK:   [[CAST2:%.*]] = bitcast i32* [[CAST1]] to i8*
+  // CHECK:   [[CALL:%.*]] = call arm_aapcscc i32 @__atomic_load_4(i8* [[CAST2]], i32 5)
+  // CHECK:   store i32 [[CALL]], i32* [[ATOMIC_RES]], align 4
+  // CHECK:   [[RES:%.*]] = load i32, i32* [[ATOMIC_RES]], align 4
+  // CHECK:   ret i32 [[RES]]
+  (void)__atomic_is_lock_free(1, addr);
+  return (int)__c11_atomic_load((_Atomic int *)addr, 5);
+}
+
 struct Empty {};
 
 struct Empty test_empty_struct_load(_Atomic(struct Empty)* em

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 8 inline comments as done.
aaron.ballman added inline comments.



Comment at: lib/Sema/SemaType.cpp:7604-7608
+  // When instantiating a class template and reaching an atomic type, the check
+  // for type completeness should occur on the underlying type and not the
+  // atomic type itself (which is always incomplete).
+  if (inTemplateInstantiation() && T->isAtomicType())
+T = cast(T)->getValueType();

rsmith wrote:
> This function is duplicating the work of finding the type to complete, which 
> was already done by `isIncompleteType`. Rather than unwrapping `T` here...
Thank you for the explanation!


https://reviews.llvm.org/D46112



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

[PATCH] D46894: [Attr] Don't print implicit attributes

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D46894



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

[PATCH] D46905: [Attr] Don't print fake MSInheritance argument

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D46905



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

[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Can you roll back the changes in MatchVerifier.h? Phab is telling me that the 
only changes are to whitespace.




Comment at: lib/AST/DeclBase.cpp:1353
 
+static bool shouldBeHidden(NamedDecl *D);
+

Rather than make a hanging forward declaration, can you just move the 
definition of this function here?



Comment at: lib/AST/DeclBase.cpp:1387
+// lookup.  E.g. template specializations are skipped.
+if (shouldBeHidden(ND)) return;
+

The `return` should be on its own line per our usual conventions.


Repository:
  rC Clang

https://reviews.llvm.org/D46835



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

[PATCH] D46439: [Sema] Fix incorrect packed aligned structure layout

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D46439



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

[PATCH] D45702: [clang-tidy] Add a new check, readability-simplify-subscript-expr, that simplifies subscript expressions.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D45702#1097294, @shuaiwang wrote:

> Addressed review comments.


This patch was approved; do you need someone to commit this for you?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45702



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

[PATCH] D45702: [clang-tidy] Add a new check, readability-simplify-subscript-expr, that simplifies subscript expressions.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Committed in r332519, thank you for the patch!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45702



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

[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D46835



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

[PATCH] D45679: [clang-tidy] Add ExprMutationAnalyzer, that analyzes whether an expression is mutated within a statement.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:21-25
+  for (const auto *Init : Node.capture_inits()) {
+if (Init == E)
+  return true;
+  }
+  return false;

This can be replaced with `return llvm::find(Node.capture_inits(), E);`



Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:34-38
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+cxxTypeidExpr;
+
+const ast_matchers::internal::VariadicDynCastAllOfMatcher
+cxxNoexceptExpr;

I think these should be exposed as public AST matchers, as they're both 
generally useful. It can be done in a follow-up patch, or done before landing 
this patch, either is fine by me.



Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:67
+
+bool ExprMutationAnalyzer::isUnevaluated(const Expr *Exp) {
+  return selectFirst(

What about other unevaluated expressions, like `typeof`, `_Generic`, etc? You 
should search for `ExpressionEvaluationContext::Unevaluated` in Clang to see 
where we set up an unevaluated expression evaluation context to find all of the 
situations.



Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:72
+ findAll(expr(equalsNode(Exp),
+  anyOf(hasAncestor(typeLoc()),
+hasAncestor(expr(anyOf(

What is this trying to match with the `typeLoc()` matcher?



Comment at: clang-tidy/utils/ExprMutationAnalyzer.cpp:74-75
+hasAncestor(expr(anyOf(
+unaryExprOrTypeTraitExpr(),
+cxxTypeidExpr(), cxxNoexceptExpr())
+ .bind("expr")),

I think these are only approximations to testing whether it's unevaluated. For 
instance, won't this match these expressions, despite them being evaluated?
```
// C++
#include 

struct B {
  virtual ~B() = default;
};

struct D : B {};

B& get() {
  static B *b = new D;
  return *b;
}

void f() {
  (void)typeid(get()); // Evaluated, calls get()
}

/* C99 and later */
#include 

void f(size_t n) {
  (void)sizeof(int[++n]); // Evaluated, increments n
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45679



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

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35
+  hasSourceExpression(IsFloatExpr),
+  unless(hasParent(castExpr(
+ .bind("cast"),

I believe this code will not diagnose under this check -- is that intended as a 
way to silence the check?
```
i += (double)0.5;
```



Comment at: 
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:10-11
+
+This rule is part of the "Expressions and statements" profile of the C++ Core
+Guidelines, corresponding to rule ES.46. See
+

More exposition is needed because this is only a very small part of the core 
guideline. You should be explicit about where we deviate (or what we support, 
depending on which is the clearest exposition).



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:81
+
+}  // namespace floats

What should happen in cases like the following:
```
template 
void f(T1 one, T2 two) {
  one += two;
}

void g() {
  f(1, 2);
  f(1, .5);
}

#define DERP(i, j) (i += j)

void h() {
  int i = 0;
  DERP(1, 2);
  DERP(i, .5);
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



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

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35
+  hasSourceExpression(IsFloatExpr),
+  unless(hasParent(castExpr(
+ .bind("cast"),

courbet wrote:
> aaron.ballman wrote:
> > I believe this code will not diagnose under this check -- is that intended 
> > as a way to silence the check?
> > ```
> > i += (double)0.5;
> > ```
> Did you mean `(int)0.5` ?
> 
> Yes, the user essentially told us they knew what they were doing. I've added 
> an explicit test for this.
I truly meant `(double)0.5` -- where the cast has no impact on the narrowing 
conversion, but the check still doesn't diagnose because there's an explicit 
cast present. Should the check be checking for explicit casts to the narrowed 
type?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



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

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Aaron Ballman via Phabricator via cfe-commits]

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

I think this generally LGTM. You should wait a bit to see if @alexfh has any 
other concerns.




Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35
+  hasSourceExpression(IsFloatExpr),
+  unless(hasParent(castExpr(
+ .bind("cast"),

courbet wrote:
> aaron.ballman wrote:
> > courbet wrote:
> > > aaron.ballman wrote:
> > > > I believe this code will not diagnose under this check -- is that 
> > > > intended as a way to silence the check?
> > > > ```
> > > > i += (double)0.5;
> > > > ```
> > > Did you mean `(int)0.5` ?
> > > 
> > > Yes, the user essentially told us they knew what they were doing. I've 
> > > added an explicit test for this.
> > I truly meant `(double)0.5` -- where the cast has no impact on the 
> > narrowing conversion, but the check still doesn't diagnose because there's 
> > an explicit cast present. Should the check be checking for explicit casts 
> > to the narrowed type?
> OK, then that's fine (I added a test for that for): there is an explicit cast 
> to double, but then the compiler generates an extra cast to int on top, which 
> triggers.
Ah, excellent, thank you!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

Adding Richard as he has more background in this area. In general, I think it's 
okay - if we invalidate too often, we do lose some performance, but the 
correctness should still be there. I'll let Richard provide the final sign-off, 
however.


https://reviews.llvm.org/D46940



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

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D44539#1106152, @rjmccall wrote:

> This was approved by the Objective-C language group as a default-off warning.


We usually do not expose new default-off warnings because experience shows that 
they rarely ever get enabled by users. If the ObjC group doesn't think this 
should be on by default, I wonder if it should be included in Clang at all.


Repository:
  rC Clang

https://reviews.llvm.org/D44539



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

[PATCH] D45835: Add new driver mode for dumping compiler options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ping.


https://reviews.llvm.org/D45835



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

[PATCH] D47122: [clang-tidy] SimplifyBoolenExpr doesn't add parens if unary negotiation is of ExprWithCleanups type

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM, with a small nit.




Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:78
   E = E->IgnoreImpCasts();
+  if (auto *EC = dyn_cast(E))
+E = EC->getSubExpr();

`const auto *`, please.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47122



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

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D44539#1106802, @rjmccall wrote:

> In https://reviews.llvm.org/D44539#1106443, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D44539#1106152, @rjmccall wrote:
> >
> > > This was approved by the Objective-C language group as a default-off 
> > > warning.
> >
> >
> > We usually do not expose new default-off warnings because experience shows 
> > that they rarely ever get enabled by users. If the ObjC group doesn't think 
> > this should be on by default, I wonder if it should be included in Clang at 
> > all.
>
>
> That's a fair question to ask.  In this case, I'm in favor of adding it 
> because we have evidence of there being a substantial set of users who would 
> enable it enthusiastically.


Okay, that works for me. Thank you for verifying.


Repository:
  rC Clang

https://reviews.llvm.org/D44539



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

> The warning is off by default.

We typically do not add off-by-default warnings because experience has shown 
the rarely get enabled in practice. Can you explain a bit more about why this 
one is off by default?


Repository:
  rC Clang

https://reviews.llvm.org/D47157



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

[PATCH] D47200: [Sema] Add tests for weak functions

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D47200



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

[PATCH] D46805: If some platforms do not support an attribute, we should exclude the platform

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

@rsmith -- do the object file formats listed look correct to you?




Comment at: include/clang/Basic/Attr.td:322
+def TargetSupportsAlias : TargetSpec {
+  let ObjectFormats = ["COFF", "ELF", "Wasm"];
+}

Did you verify that Wasm supports the alias attribute? If it is supported, it 
might be nice to add a test to `CodeGen/alias.c` to demonstrate it. Similar for 
COFF.



Comment at: test/Sema/attr-alias-has.c:5
+// RUN: %clang_cc1 -triple wasm32-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple wasm64-unknown-unknown -fsyntax-only -verify %s
+

I'd like to see a test that the "attribute not supported on target" diagnostic 
is being generated. I'd recommend something along these lines:
```
void g() {}
void f() __attribute__((alias("g")));
#if !__has_attribute(alias)
// expected-error@-2{{expected diagnostic text}}
#else
// expected-no-diagnostics
#endif
```


Repository:
  rC Clang

https://reviews.llvm.org/D46805



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

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is relaxing `-Wformat` and making users instead write `-Wformat-pedantic` 
to get the strong guarantees, which is the opposite of what I thought the 
consensus was. Have I misunderstood something?




Comment at: include/clang/Analysis/Analyses/FormatString.h:263-265
+  ArgType(Kind k = UnknownTy, const char *n = nullptr) : K(k), Name(n) {}
+  ArgType(QualType t, const char *n = nullptr) : K(SpecificTy), T(t), Name(n) 
{}
+  ArgType(CanQualType t) : K(SpecificTy), T(t) {}

If we're going to update this, can you fix the parameter names to be in line 
with the coding conventions?



Comment at: lib/Sema/SemaChecking.cpp:6597-6599
+  const analyze_printf::ArgType::MatchKind match =
+  AT.matchesType(S.Context, ExprTy);
+  bool pedantic = match == analyze_printf::ArgType::NoMatchPedantic;

These identifiers also need some love to match the coding conventions. Happens 
elsewhere in this patch as well.


Repository:
  rC Clang

https://reviews.llvm.org/D47290



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D47157#1107210, @bruno wrote:

> > See also PR22165.
>
> Nice, seems related to this indeed. Are you aware of any development along 
> those lines in clang-tidy? We would like this to be part of clang and be used 
> as part of the normal compiler workflow, it can surely be as well part of any 
> clang-tidy related include guidelines in the future.
>
> >> The warning is off by default.
> > 
> > We typically do not add off-by-default warnings because experience has 
> > shown the rarely get enabled in practice. Can you explain a bit more about 
> > why this one is off by default?
>
> Right. I believe this is going to be used in practice, the reason I'm adding 
> it involves some user demand for such warning. Such quoted include use in 
> frameworks happen often and we would like a smooth transition to happen here 
> (e.g. do not initially affect -Werror users). If it proves worth it, we can 
> migrate to on by default in the future. It wouldn't be a problem if we have 
> it on by default on open source and disable by default downstream, but I 
> rather be consistent.


Consistency would be nice, but at the same time, I don't see a good metric for 
when we'd know it's time to switch it to being on by default. I'm worried that 
it'll remain off by default forever simply because no one thinks to go turn it 
on (because it's silent by default). Perhaps on-by-default here and 
off-by-default downstream would be the better approach, or do you think this 
would be too disruptive to enable by default anywhere?




Comment at: include/clang/Basic/DiagnosticLexKinds.td:713
+def warn_quoted_include_in_framework_header : Warning<
+  "double-quoted include \"%0\" in framework header, expected system style 
 include"
+  >, InGroup, DefaultIgnore;

80-col limit?

Also, I'd probably drop "system style" and reword slightly to:

`"double-quoted include \"%0\" in framework header, expected angle-bracketed 
include <%0> instead"`



Comment at: lib/Lex/HeaderSearch.cpp:753-754
+  IncluderAndDir.second->getName()))
+Diags.Report(IncludeLoc,
+ diag::warn_quoted_include_in_framework_header)
+<< Filename;

This seems like a good place for a fix-it to switch the include style. Is there 
a reason to not do that work for the user?



Comment at: lib/Lex/HeaderSearch.cpp:873-874
+!FoundByHeaderMap)
+  Diags.Report(IncludeLoc, diag::warn_quoted_include_in_framework_header)
+  << Filename;
+

Similar here.


Repository:
  rC Clang

https://reviews.llvm.org/D47157



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

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D45686#1109295, @dstenb wrote:

> Ping.
>
> We have added a lit reproducer for this now in clang-tools-extra: 
> https://reviews.llvm.org/D47251.


The above should be rolled into this patch as the test case verifying the 
behavioral changes.


https://reviews.llvm.org/D45686



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

[PATCH] D47290: [Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D47290#1113412, @jfb wrote:

> In https://reviews.llvm.org/D47290#1113365, @aaron.ballman wrote:
>
> > This is relaxing `-Wformat` and making users instead write 
> > `-Wformat-pedantic` to get the strong guarantees, which is the opposite of 
> > what I thought the consensus was. Have I misunderstood something?
>
>
> From Shoaib's email:
>
> > Based on all the discussion so far, I think the consensus is that we don't 
> > want to relax -Wformat by default, but an additional relaxed option would 
> > be fine. That works for me (where I can just switch my codebases to use the 
> > new warning option), but it wouldn't handle JF Bastien's use case, so he 
> > would still want to pursue the relaxations specific to Apple platforms.
>
> This patch is specific to the Darwin platform, as proposed in Alex' original 
> patch and as I promised to do in the cfe-dev thread. Shoaib's follow-up would 
> be different and do what I think you're referring to.


There were several people on the thread asking to not relax -Wformat for any 
target, but instead wanted a separate warning flag to perform a more relaxed 
check.

http://lists.llvm.org/pipermail/cfe-dev/2018-May/057938.html
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058030.html
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058039.html


Repository:
  rC Clang

https://reviews.llvm.org/D47290



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

[PATCH] D45835: Add new driver mode for dumping compiler options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ping.


https://reviews.llvm.org/D45835



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

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 148753.
aaron.ballman marked 4 inline comments as done.
aaron.ballman added a comment.

Corrected some of the review comments.


https://reviews.llvm.org/D46112

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/Type.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaType.cpp
  test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
  test/CodeGen/c11atomics.c
  test/Sema/atomic-type.c
  test/SemaCXX/atomic-type.cpp
  test/SemaObjC/arc.m

Index: test/SemaObjC/arc.m
===
--- test/SemaObjC/arc.m
+++ test/SemaObjC/arc.m
@@ -410,7 +410,7 @@
   [v test16_6: 0];
 }
 
-@class Test17; // expected-note 2{{forward declaration of class here}}
+@class Test17; // expected-note 3{{forward declaration of class here}}
 @protocol Test17p
 - (void) test17;
 + (void) test17;
Index: test/SemaCXX/atomic-type.cpp
===
--- test/SemaCXX/atomic-type.cpp
+++ test/SemaCXX/atomic-type.cpp
@@ -94,3 +94,15 @@
 bool PR21836(_Atomic(int) *x) {
 return *x;
 }
+
+struct T;
+
+void f(_Atomic T *t);
+
+struct T { int a, b; };
+
+void f(_Atomic T *t) {
+  T t2;
+  (void)__atomic_load(t, &t2, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(T) *' invalid)}}
+  (void)__c11_atomic_load(t, 5);
+}
Index: test/Sema/atomic-type.c
===
--- test/Sema/atomic-type.c
+++ test/Sema/atomic-type.c
@@ -16,7 +16,23 @@
 extern _Atomic(int (*)(int(*)[10], int(*)[10])) mergetest;
 
 _Atomic(int()) error1; // expected-error {{_Atomic cannot be applied to function type}}
-_Atomic(struct ErrorS) error2; // expected-error {{_Atomic cannot be applied to incomplete type}} expected-note {{forward declaration}}
+_Atomic(struct ErrorS) error2; // expected-error {{tentative definition has type '_Atomic(struct ErrorS)' that is never completed}} \
+   // expected-note {{forward declaration of 'struct ErrorS'}}
 _Atomic(int[10]) error3; // expected-error {{_Atomic cannot be applied to array type}}
 _Atomic(const int) error4; // expected-error {{_Atomic cannot be applied to qualified type}}
 _Atomic(_Atomic(int)) error5; // expected-error {{_Atomic cannot be applied to atomic type}}
+
+void g(_Atomic void *ptr);
+void h(_Atomic struct Incomplete *ptr); // expected-warning {{declaration of 'struct Incomplete' will not be visible outside of this function}}
+
+void test_atomic_void_ptr(_Atomic void *addr) {
+  int i;
+  (void)__atomic_load(addr, &i, 5); // expected-error {{address argument to atomic operation must be a pointer to a trivially-copyable type ('_Atomic(void) *' invalid)}}
+  (void)__c11_atomic_load(addr, 5); // expected-error {{invalid use of incomplete type 'void'}}
+  _Static_assert(__atomic_is_lock_free(1, addr), "");
+}
+
+typedef struct S S; // expected-note {{forward declaration of 'struct S'}}
+void test_atomic_incomplete_struct_assignment(_Atomic S *s, _Atomic S *s2) {
+  *s = *s2; // expected-error {{incomplete type '_Atomic(S)' is not assignable}}
+}
Index: test/CodeGen/c11atomics.c
===
--- test/CodeGen/c11atomics.c
+++ test/CodeGen/c11atomics.c
@@ -61,7 +61,7 @@
   // we have to generate an atomic add, which returns the old value, and then a
   // non-atomic add.
   // CHECK: atomicrmw add i32* @i, i32 1 seq_cst
-  // CHECK: add i32 
+  // CHECK: add i32
   ++i;
   // CHECK: atomicrmw add i64* @l, i64 1 seq_cst
   // CHECK: add i64
@@ -475,6 +475,25 @@
   return __c11_atomic_compare_exchange_strong(addr, desired, *new, 5, 5);
 }
 
+int test_atomic_void_ptr(_Atomic void *addr) {
+  // CHECK-LABEL: @test_atomic_void_ptr(
+  // CHECK:   [[ADDR_ARG:%.*]] = alloca { i8, [1 x i8] }*, align 4
+  // CHECK:   [[ATOMIC_RES:%.*]] = alloca i32, align 4
+  // CHECK:   store { i8, [1 x i8] }* %addr, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE_VP:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[ADDR_LOCK_FREE:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR_LOCK_FREE_VP]] to i8*
+  // CHECK:   call arm_aapcscc i32 @__atomic_is_lock_free(i32 1, i8* [[ADDR_LOCK_FREE]])
+  // CHECK:   [[ADDR:%.*]] = load { i8, [1 x i8] }*, { i8, [1 x i8] }** [[ADDR_ARG]], align 4
+  // CHECK:   [[CAST1:%.*]] = bitcast { i8, [1 x i8] }* [[ADDR]] to i32*
+  // CHECK:   [[CAST2:%.*]] = bitcast i32* [[CAST1]] to i8*
+  // CHECK:   [[CALL:%.*]] = call arm_aapcscc i32 @__atomic_load_4(i8* [[CAST2]], i32 5)
+  // CHECK:   store i32 [[CALL]], i32* [[ATOMIC_RES]], align 4
+  // CHECK:   [[RES:%.*]] = load i32, i32* [[ATOMIC_RES]], align 4
+  // CHECK:   ret i32 [[RES]]
+  (void)__atomic_is_lock_free(1, addr);
+  return (int)__c11_atomic_load((_Atomic int *)addr, 5);
+}
+
 struct Empty {};
 
 struct Empty test_empty_struct_load(_Atomic(struct Empty

[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D46112#1098243, @rsmith wrote:

> In https://reviews.llvm.org/D46112#1096893, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D46112#1091981, @efriedma wrote:
> >
> > > Also needs some test coverage for atomic operations which aren't calls, 
> > > like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; 
> > > };".
> >
> >
> > Thank you for pointing this out -- that uncovered an issue where we were 
> > not properly diagnosing the types as being incomplete. I've added a new 
> > test case and rolled the contents of Sema\atomic-type.cpp (which I added in 
> > an earlier patch) into SemaCXX\atomic-type.cpp (which already existed and I 
> > missed it).
>
>
> I don't think this has sufficiently addressed the comment. Specifically, for 
> a case like:
>
>   struct NotTriviallyCopyable { NotTriviallyCopyable(const 
> NotTriviallyCopyable&); };
>   void f(_Atomic NotTriviallyCopyable *p) { *p = *p; }
>
>
> ... we should reject the assignment, because it would perform an atomic 
> operation on a non-trivially-copyable type. It would probably suffice to 
> check the places where we form an `AtomicToNonAtomic` or `NonAtomicToAtomic` 
> conversion.
>
> In passing, I noticed that this crashes Clang (because we fail to create an 
> lvalue-to-rvalue conversion for `x`):
>
>   struct X {};  
>   void f(_Atomic X *p, X x) { *p = x; }
>
>
> This will likely get in the way of your test cases unless you fix it :)


It only gets in the way for C++ whereas my primary concern for this patch is C. 
I tried spending a few hours on this today and got nowhere -- we have a lot of 
FIXME comments surrounding atomic type qualifiers in C++. I've run out of time 
to be able to work on this patch and may have to abandon it. I'd hate to lose 
the forward progress already made, so I'm wondering if the C bits are 
sufficiently baked that even more FIXMEs around atomics in C++ would suffice?


https://reviews.llvm.org/D46112



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

[PATCH] D47435: Add __builtin_signbit semantic checking

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rengolin, chatur01, rsmith.
aaron.ballman added inline comments.



Comment at: test/Sema/builtins.c:228
 // expected-note {{change size argument to 
be the size of the destination}}
-   
+
 __builtin___strlcat_chk(buf, b, sizeof(b), __builtin_object_size(buf, 
0)); // expected-warning {{size argument in '__builtin___strlcat_chk' call 
appears to be size of the source; expected the size of the destination}} \

I'll revert this sneaky whitespace either as part of the commit, or as an 
updated patch if one is required.


r242675 changed the signature for the signbit builtin but did not introduce 
proper semantic checking to ensure the arguments are as-expected. This lead to 
regressions like the one reported in PR28172 where codegen would crash. This 
patch addresses this by properly grouping the signbit builtins along with the 
other fp classification builtins.


Repository:
  rC Clang

https://reviews.llvm.org/D47435

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/builtins.c


Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -225,7 +225,7 @@
 
 strlcat(buf, b, sizeof(b)); // expected-warning {{size argument in 
'strlcat' call appears to be size of the source; expected the size of the 
destination}} \
 // expected-note {{change size argument to 
be the size of the destination}}
-   
+
 __builtin___strlcat_chk(buf, b, sizeof(b), __builtin_object_size(buf, 
0)); // expected-warning {{size argument in '__builtin___strlcat_chk' call 
appears to be size of the source; expected the size of the destination}} \

// expected-note {{change size argument to be the size of the destination}} 
\

   // expected-warning {{'__builtin___strlcat_chk' will always overflow 
destination buffer}}
@@ -253,3 +253,17 @@
   __sync_fetch_and_add(ptr, 1); // expected-error{{address argument to atomic 
builtin cannot be const-qualified ('const int *' invalid)}}
   __atomic_fetch_add(ptr, 1, 0);  // expected-error {{address argument to 
atomic operation must be a pointer to non-const type ('const int *' invalid)}}
 }
+
+void test22(void) {
+  (void)__builtin_signbit(); // expected-error{{too few arguments to function 
call, expected 1, have 0}}
+  (void)__builtin_signbit(1.0, 2.0, 3.0); // expected-error{{too many 
arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbit(1); // expected-error {{floating point 
classification requires argument of floating point type (passed in 'int')}}
+
+  (void)__builtin_signbitf(); // expected-error{{too few arguments to function 
call, expected 1, have 0}}
+  (void)__builtin_signbitf(1.0, 2.0, 3.0); // expected-error{{too many 
arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbitf(1);
+
+  (void)__builtin_signbitl(); // expected-error{{too few arguments to function 
call, expected 1, have 0}}
+  (void)__builtin_signbitl(1.0, 2.0, 3.0); // expected-error{{too many 
arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbitl(1);
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -919,6 +919,9 @@
   case Builtin::BI__builtin_isinf_sign:
   case Builtin::BI__builtin_isnan:
   case Builtin::BI__builtin_isnormal:
+  case Builtin::BI__builtin_signbit:
+  case Builtin::BI__builtin_signbitf:
+  case Builtin::BI__builtin_signbitl:
 if (SemaBuiltinFPClassification(TheCall, 1))
   return ExprError();
 break;


Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -225,7 +225,7 @@
 
 strlcat(buf, b, sizeof(b)); // expected-warning {{size argument in 'strlcat' call appears to be size of the source; expected the size of the destination}} \
 // expected-note {{change size argument to be the size of the destination}}
-
+
 __builtin___strlcat_chk(buf, b, sizeof(b), __builtin_object_size(buf, 0)); // expected-warning {{size argument in '__builtin___strlcat_chk' call appears to be size of the source; expected the size of the destination}} \
// expected-note {{change size argument to be the size of the destination}} \
    // expected-warning {{'__builtin___strlcat_chk' will always overflow destination buffer}}
@@ -253,3 +253,17 @@
   __sy

[PATCH] D47435: Add __builtin_signbit semantic checking

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/Sema/builtins.c:228
 // expected-note {{change size argument to 
be the size of the destination}}
-   
+
 __builtin___strlcat_chk(buf, b, sizeof(b), __builtin_object_size(buf, 
0)); // expected-warning {{size argument in '__builtin___strlcat_chk' call 
appears to be size of the source; expected the size of the destination}} \

I'll revert this sneaky whitespace either as part of the commit, or as an 
updated patch if one is required.


Repository:
  rC Clang

https://reviews.llvm.org/D47435



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

[PATCH] D45835: Add new driver mode for dumping compiler options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 3 inline comments as done.
aaron.ballman added inline comments.



Comment at: test/Frontend/compiler-options-dump.cpp:3
+// RUN: %clang_cc1 -compiler-options-dump -std=c++17 %s -o - | FileCheck %s 
--check-prefix=CXX17
+// RUN: %clang_cc1 -compiler-options-dump -std=c99 -ffast-math -x c %s -o - | 
FileCheck %s --check-prefix=C99
+

hfinkel wrote:
> You don't need -ffast-math here I presume.
Correct, I'll drop from the commit (or in the next patch).



Comment at: test/Frontend/compiler-options-dump.cpp:15
+// CXX17: "extensions"
+// CXX17: "cxx_range_for" : true
+

hfinkel wrote:
> cxx_range_for is both a feature and an extension?
Correct. The way we implement __has_feature is to return true only if the -std 
line supports the feature, and __has_extension will return true if 
__has_feature returns true or if the extension is enabled.


https://reviews.llvm.org/D45835



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

[PATCH] D38209: [Sema] Correct nothrow inherited by noexcept

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'm not certain we have the semantics of `__declspec(nothrow)` exactly correct 
-- a simple test shows that `__attribute__((nothrow))`, `__declspec(nothrow)`, 
and `noexcept(true)` are subtly different.

When I run this with MSVC 2017, the terminate handler is not called and the 
application continues rather than terminates, but Clang calls the terminate 
handler.

  #include 
  #include 
  
  __declspec(nothrow) void f() { throw 1; }
  
  int main() {
std::set_terminate([]() {
  std::cout << "terminate called" << std::endl;
  std::abort();
});
  
f();
  }

However, in this case terminate is called using GCC 6.3 and Clang.

  #include 
  #include 
   
  __attribute__((nothrow)) void f() { throw 1; }
   
  int main() {
std::set_terminate([]() {
  std::cout << "terminate called" << std::endl;
  std::abort();
});
   
f();
  }

For your test case: GCC accepts (with __attribute__), MSVC accepts (with 
__declspec), and EDG rejects `Derived() noexcept = default` because of 
incompatible exception specs. I think it's probably reasonable for us to accept 
despite the slight semantic differences.

@STL_MSFT -- do you think `__declspec(nothrow)` calling the terminate handler 
in Clang is a bug?




Comment at: lib/Sema/SemaDeclCXX.cpp:170
 
+  if (EST == EST_None && Method->hasAttr()) {
+EST = EST_BasicNoexcept;

Elide the braces.



Comment at: test/SemaCXX/nothrow-as-noexcept-ctor.cpp:3
+
+
+// expected-no-diagnostics

You can remove the spurious newline from the test.


https://reviews.llvm.org/D38209



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

[PATCH] D38209: [Sema] Correct nothrow inherited by noexcept

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

In https://reviews.llvm.org/D38209#880553, @STL_MSFT wrote:

> > do you think `__declspec(nothrow)` calling the terminate handler in Clang 
> > is a bug?
>
> It's certainly a behavior difference with potential performance impact, 
> although I don't think it can be viewed as a bug, strictly speaking. MSVC 
> treats `__declspec(nothrow)` as an optimization request, with undefined 
> behavior if it's violated. Termination is definitely one of the possible 
> results of undefined behavior.
>
> We've recently had to slightly rethink our EH strategy in light of C++17's 
> addition of noexcept into the type system, and the removal of dynamic 
> exception specifications (and the change in semantics to throw()). MSVC's 
> /EHsc makes this extra fun. If you're interested, I can put you in contact 
> with the compiler dev who recently made those changes.


That might be helpful. I'm mostly interested in whether `__declspec(nothrow)` 
is intended to be part of the type system in the same way `noexcept` specifiers 
are.

@erichkeane -- can you see whether `__attribute__((nothrow))` is part of the 
type system as far as GCC is concerned (in C++17 mode, assuming GCC has 
implemented that functionality already)?


https://reviews.llvm.org/D38209



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

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D37436#871117, @aaron.ballman wrote:

> Updated based on review feedback.
>
> - Rename CAttributes to DoubleSquareBracketAttributes
> - Added Objective-C test cases to demonstrate no regressed behavior (based on 
> a use-case pointed out during review)
> - Fixed regression with GNU-style attributes in enumerations
>
>   I still need to rename the cc1 flag, pending agreement on the name.


Ping -- the last outstanding issue that I'm aware of is the cc1 flag name, do 
you have a preference there, Richard? If not, I'll be going with the verbose 
`-fdouble-square-bracket-attributes`.


https://reviews.llvm.org/D37436



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

[PATCH] D38270: [clang] Add getUnsignedPointerDiffType method

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

> The code for printf diagnostics + new tests are supposed to be added by a 
> separate diff.

I think you should combine that diff and this one into a single patch, 
otherwise this patch is untestable.




Comment at: include/clang/AST/ASTContext.h:1492-1493
 
+  /// \brief Return the unique unsigned counterpart of 
+  /// "ptrdiff_t" integer type.
+  QualType getUnsignedPointerDiffType() const;

Wrap the comment, and you might want to refer to C11 7.21.6.1p7 as a 
justification for why this is needed.


Repository:
  rL LLVM

https://reviews.llvm.org/D38270



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

[PATCH] D38202: Add Documentation to attribute-nothrow. Additionally, limit to functions.

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D38202



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

[PATCH] D38209: [Sema] Correct nothrow inherited by noexcept

[Aaron Ballman via Phabricator via cfe-commits]

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

This is somewhat complicated in that noexcept, throw(), and 
__attribute__(())/__declspec no throw are all synonyms for one another except 
for the type system effect differences between __attribute__ and __declspec, 
but I think our behavior here is reasonable.


https://reviews.llvm.org/D38209



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

[PATCH] D38205: [Sema] Warn on attribute nothrow conflicting with language specifiers

[Aaron Ballman via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D38205#882737, @erichkeane wrote:

> changes that were suggested by @aaron.ballman
>
> It DOES warn on the template correctly, however I'm not thrilled that it 
> misses the 'warning' info.  Any idea how I can get that information to give 
> that 'note'?  I'd like an "in instantiation of template ..." type note.


That's usually automatic. You might want to look at 
`Sema::EmitCurrentDiagnostic()` to see what's up, it's `PrintContextStack()` 
that does the magic.

The diagnostics LGTM as-is, but further improvement isn't discouraged.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1411
+def warn_nothrow_attr_disagrees_with_exception_specification
+: ExtWarn<"Attribute nothrow ignored, it disagrees with language specified 
"
+  "exception specification">,

aaron.ballman wrote:
> How about: `"attribute 'nothrow' ignored due to conflicting exception 
> specification"`
Close, but not quite. The warning should not be capitalized and it's missing 
single quotes around `nothrow`.


https://reviews.llvm.org/D38205



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

[PATCH] D38270: [clang] Add getUnsignedPointerDiffType method

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/FixIt/format.m:256
+  // see the comment in PrintfSpecifier::fixType in PrintfFormatString.cpp.
+}
+

Can you add some positive tests that show "%t" and "%tu" working properly 
without warnings?


Repository:
  rL LLVM

https://reviews.llvm.org/D38270



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

[PATCH] D38342: [C++] Parse (sub) postfix expression after boolean literal

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/CXX/expr/expr.post/expr.sub/p1.cpp:3-6
+void pr34273() {
+  char Normal = "clang"[true]; // expected-no-diagnostics
+  char Special = true["clang"]; // expected-no-diagnostics
+}

I think this test should be added to test/Parser/ instead of CXX; this does not 
really appertain to [expr.sub]p1, which is more about the semantics than the 
parsing.



Comment at: test/CXX/expr/expr.post/expr.sub/p1.cpp:4-5
+void pr34273() {
+  char Normal = "clang"[true]; // expected-no-diagnostics
+  char Special = true["clang"]; // expected-no-diagnostics
+}

The "expected-no-diagnostics" comment should be right below the RUN line, and 
you only need it once.


https://reviews.llvm.org/D38342



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

[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D36836#883214, @lebedev.ri wrote:

> @alexfh please specify, is that enough or not?


I think it's sufficient, but we should put that information somewhere pertinent 
rather than just the review. In the past, we've put something into LICENSE.TXT 
to identify that we're in the clear, and that might be appropriate here as 
well. What do you think, @alexfh?


Repository:
  rL LLVM

https://reviews.llvm.org/D36836



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

[PATCH] D38284: [clang-tidy] Fix google-readability-namespace-comments handling of C++17 nested namespaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:86
+  SourceLocation LBracketLocation = ND->getLocation();
+  auto NestedNamespaceBegin = LBracketLocation;
+

Do not use `auto` here (the type is not spelled out in the initialization, 
despite being relatively obvious. Same below.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:90
+  // then bar instead of a single match. So if we got a nested namespace we 
have
+  // to skip the next ones
+  for (const auto &i : Ends) {

Missing a period at the end of the sentence.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:91
+  // to skip the next ones
+  for (const auto &i : Ends) {
+if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin, i)) {

`i` does not meet the usual naming conventions; you might want to pick a 
slightly better name.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:92
+  for (const auto &i : Ends) {
+if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin, i)) {
+  return;

You can elide the braces from the `if` statement.



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:109
+
+  if (IsNested) {
+Ends.push_back(LBracketLocation);

Elide braces



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:137
 
-  // Check if the namespace in the comment is the same.
-  if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) ||
-  (ND->getNameAsString() == NamespaceNameInComment &&
-   Anonymous.empty())) {
+  // C++17 nested namespace
+  if (IsNested && NestedNamespaceName == NamespaceNameInComment) {

Missing period in the comment



Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:140-141
+return;
+  } // Check if the namespace in the comment is the same.
+  else if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) 
||
+   (ND->getNameAsString() == NamespaceNameInComment &&

I'd move the comments inside of the braces (the braces can be elided, but if 
the comments are in the body instead of out of it, I think they're fine to 
leave).



Comment at: clang-tidy/readability/NamespaceCommentCheck.h:37
   const unsigned SpacesBeforeComments;
+  llvm::SmallVector Ends;
 };

Why 7?



Comment at: test/clang-tidy/google-readability-nested-namespace-comments.cpp:6
+   
+   //so that namespace is not empty
+   void f();

Comment should start with a capital letter and end with punctuation.



Comment at: 
test/clang-tidy/google-readability-nested-namespace-comments.cpp:8-14
+   
+   
+   
+   
+   
+   
+   

Can remove the extra whitespace



Comment at: test/clang-tidy/google-readability-nested-namespace-comments.cpp:23
+// CHECK-FIXES: }  // namespace n1::n2
\ No newline at end of file


Please make sure there's a newline at the end of the file.


https://reviews.llvm.org/D38284



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

[PATCH] D38342: [C++] Parse (sub) postfix expression after boolean literal

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D38342#883872, @Rakete wrote:

> - Moved test to test/CXX/
>
>   Do I actually need to -verify the test if no diagnostics are expected?
>
>   Thanks @aaron.ballman


Yes, you need to make sure that verify is on the RUN line. The existing test is 
broken like that (good catch). You should add -verify to the run line and // 
expected-no-diagnostics below it.


https://reviews.llvm.org/D38342



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

[PATCH] D38342: [C++] Parse (sub) postfix expression after boolean literal

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM! Do you need me to commit on your behalf?


https://reviews.llvm.org/D38342



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

[PATCH] D38342: [C++] Parse (sub) postfix expression after boolean literal

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Committed in r314463.

Btw, if you're not in the IRC channel (#llvm on OFTC), you should hang out in 
there to make sure no build bots break due to the commit.


https://reviews.llvm.org/D38342



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

[PATCH] D38270: [clang] Add getUnsignedPointerDiffType method

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rL LLVM

https://reviews.llvm.org/D38270



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