[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for the review! :)


Repository:
  rL LLVM

https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL291418: Add the diagnose_if attribute to clang. (authored by 
gbiv).

Changed prior to commit:
  https://reviews.llvm.org/D27424?vs=83412=83584#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27424

Files:
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Basic/DiagnosticCommonKinds.td
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Sema/Initialization.h
  cfe/trunk/include/clang/Sema/Overload.h
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/AST/ExprConstant.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Sema/SemaLookup.cpp
  cfe/trunk/lib/Sema/SemaOverload.cpp
  cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
  cfe/trunk/test/Sema/diagnose_if.c
  cfe/trunk/test/SemaCXX/diagnose_if.cpp

Index: cfe/trunk/include/clang/AST/Expr.h
===
--- cfe/trunk/include/clang/AST/Expr.h
+++ cfe/trunk/include/clang/AST/Expr.h
@@ -651,7 +651,8 @@
   /// constant.
   bool EvaluateWithSubstitution(APValue , ASTContext ,
 const FunctionDecl *Callee,
-ArrayRef Args) const;
+ArrayRef Args,
+const Expr *This = nullptr) const;
 
   /// \brief If the current Expr is a pointer, this will try to statically
   /// determine the number of bytes available where the pointer is pointing.
Index: cfe/trunk/include/clang/Sema/Overload.h
===
--- cfe/trunk/include/clang/Sema/Overload.h
+++ cfe/trunk/include/clang/Sema/Overload.h
@@ -668,6 +668,26 @@
 /// to be used while performing partial ordering of function templates.
 unsigned ExplicitCallArguments;
 
+/// The number of diagnose_if attributes that this overload triggered.
+/// If any of the triggered attributes are errors, this won't count
+/// diagnose_if warnings.
+unsigned NumTriggeredDiagnoseIfs = 0;
+
+/// Basically a TinyPtrVector that doesn't own the vector:
+/// If NumTriggeredDiagnoseIfs is 0 or 1, this is a DiagnoseIfAttr *,
+/// otherwise it's a pointer to an array of `NumTriggeredDiagnoseIfs`
+/// DiagnoseIfAttr *s.
+llvm::PointerUnion DiagnoseIfInfo;
+
+/// Gets an ArrayRef for the data at DiagnoseIfInfo. Note that this may give
+/// you a pointer into DiagnoseIfInfo.
+ArrayRef getDiagnoseIfInfo() const {
+  auto *Ptr = NumTriggeredDiagnoseIfs <= 1
+  ? DiagnoseIfInfo.getAddrOfPtr1()
+  : DiagnoseIfInfo.get();
+  return {Ptr, NumTriggeredDiagnoseIfs};
+}
+
 union {
   DeductionFailureInfo DeductionFailure;
   
@@ -732,31 +752,61 @@
 SmallVector Candidates;
 llvm::SmallPtrSet Functions;
 
-// Allocator for OverloadCandidate::Conversions. We store the first few
-// elements inline to avoid allocation for small sets.
-llvm::BumpPtrAllocator ConversionSequenceAllocator;
+// Allocator for OverloadCandidate::Conversions and DiagnoseIfAttr* arrays.
+// We store the first few of each of these inline to avoid allocation for
+// small sets.
+llvm::BumpPtrAllocator SlabAllocator;
 
 SourceLocation Loc;
 CandidateSetKind Kind;
 
-unsigned NumInlineSequences;
-llvm::AlignedCharArray
-InlineSpace;
+constexpr static unsigned NumInlineBytes =
+24 * sizeof(ImplicitConversionSequence);
+unsigned NumInlineBytesUsed;
+llvm::AlignedCharArray InlineSpace;
+
+/// If we have space, allocates from inline storage. Otherwise, allocates
+/// from the slab allocator.
+/// FIXME: It would probably be nice to have a SmallBumpPtrAllocator
+/// instead.
+template 
+T *slabAllocate(unsigned N) {
+  // It's simpler if this doesn't need to consider alignment.
+  static_assert(alignof(T) == alignof(void *),
+"Only works for pointer-aligned types.");
+  static_assert(std::is_trivial::value ||
+std::is_same::value,
+"Add destruction logic to OverloadCandidateSet::clear().");
+
+  unsigned NBytes = sizeof(T) * N;
+  if (NBytes > NumInlineBytes - NumInlineBytesUsed)
+return SlabAllocator.Allocate(N);
+  char *FreeSpaceStart = InlineSpace.buffer + NumInlineBytesUsed;
+  assert(uintptr_t(FreeSpaceStart) % alignof(void *) == 0 &&
+ "Misaligned storage!");
+
+  NumInlineBytesUsed += NBytes;
+  return 

[PATCH] D27424: Add the diagnose_if attribute to clang.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

Awesome! I'm so excited for this!


https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[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 point this out in the release notes as well.


https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> We do: 
> http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Awesome. Thanks!

> Btw, there's a question in there about late parsing that Phab won't let me 
> delete, feel free to ignore it. ;-)

OK. The answer is "yes" anyway. :)


https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 83412.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.

Addressed feedback. Thanks!


https://reviews.llvm.org/D27424

Files:
  include/clang/AST/Expr.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Initialization.h
  include/clang/Sema/Overload.h
  include/clang/Sema/Sema.h
  lib/AST/ExprConstant.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/Sema/diagnose_if.c
  test/SemaCXX/diagnose_if.cpp

Index: test/SemaCXX/diagnose_if.cpp
===
--- /dev/null
+++ test/SemaCXX/diagnose_if.cpp
@@ -0,0 +1,460 @@
+// RUN: %clang_cc1 %s -verify -fno-builtin -std=c++14
+
+#define _diagnose_if(...) __attribute__((diagnose_if(__VA_ARGS__)))
+
+namespace type_dependent {
+template 
+void neverok() _diagnose_if(!T(), "oh no", "error") {} // expected-note 4{{from 'diagnose_if'}}
+
+template 
+void alwaysok() _diagnose_if(T(), "oh no", "error") {}
+
+template 
+void alwayswarn() _diagnose_if(!T(), "oh no", "warning") {} // expected-note 4{{from 'diagnose_if'}}
+
+template 
+void neverwarn() _diagnose_if(T(), "oh no", "warning") {}
+
+void runAll() {
+  alwaysok();
+  alwaysok();
+
+  {
+void (*pok)() = alwaysok;
+pok = ;
+  }
+
+  neverok(); // expected-error{{oh no}}
+  neverok(); // expected-error{{oh no}}
+
+  {
+void (*pok)() = neverok; // expected-error{{oh no}}
+  }
+  {
+void (*pok)();
+pok = ; // expected-error{{oh no}}
+  }
+
+  alwayswarn(); // expected-warning{{oh no}}
+  alwayswarn(); // expected-warning{{oh no}}
+  {
+void (*pok)() = alwayswarn; // expected-warning{{oh no}}
+pok = ; // expected-warning{{oh no}}
+  }
+
+  neverwarn();
+  neverwarn();
+  {
+void (*pok)() = neverwarn;
+pok = ;
+  }
+}
+
+template 
+void errorIf(T a) _diagnose_if(T() != a, "oh no", "error") {} // expected-note {{candidate disabled: oh no}}
+
+template 
+void warnIf(T a) _diagnose_if(T() != a, "oh no", "warning") {} // expected-note {{from 'diagnose_if'}}
+
+void runIf() {
+  errorIf(0);
+  errorIf(1); // expected-error{{call to unavailable function}}
+
+  warnIf(0);
+  warnIf(1); // expected-warning{{oh no}}
+}
+}
+
+namespace value_dependent {
+template 
+void neverok() _diagnose_if(N == 0 || N != 0, "oh no", "error") {} // expected-note 4{{from 'diagnose_if'}}
+
+template 
+void alwaysok() _diagnose_if(N == 0 && N != 0, "oh no", "error") {}
+
+template 
+void alwayswarn() _diagnose_if(N == 0 || N != 0, "oh no", "warning") {} // expected-note 4{{from 'diagnose_if'}}
+
+template 
+void neverwarn() _diagnose_if(N == 0 && N != 0, "oh no", "warning") {}
+
+void runAll() {
+  alwaysok<0>();
+  alwaysok<1>();
+
+  {
+void (*pok)() = alwaysok<0>;
+pok = <0>;
+  }
+
+  neverok<0>(); // expected-error{{oh no}}
+  neverok<1>(); // expected-error{{oh no}}
+
+  {
+void (*pok)() = neverok<0>; // expected-error{{oh no}}
+  }
+  {
+void (*pok)();
+pok = <0>; // expected-error{{oh no}}
+  }
+
+  alwayswarn<0>(); // expected-warning{{oh no}}
+  alwayswarn<1>(); // expected-warning{{oh no}}
+  {
+void (*pok)() = alwayswarn<0>; // expected-warning{{oh no}}
+pok = <0>; // expected-warning{{oh no}}
+  }
+
+  neverwarn<0>();
+  neverwarn<1>();
+  {
+void (*pok)() = neverwarn<0>;
+pok = <0>;
+  }
+}
+
+template 
+void errorIf(int a) _diagnose_if(N != a, "oh no", "error") {} // expected-note {{candidate disabled: oh no}}
+
+template 
+void warnIf(int a) _diagnose_if(N != a, "oh no", "warning") {} // expected-note {{from 'diagnose_if'}}
+
+void runIf() {
+  errorIf<0>(0);
+  errorIf<0>(1); // expected-error{{call to unavailable function}}
+
+  warnIf<0>(0);
+  warnIf<0>(1); // expected-warning{{oh no}}
+}
+}
+
+namespace no_overload_interaction {
+void foo(int) _diagnose_if(1, "oh no", "error"); // expected-note{{from 'diagnose_if'}}
+void foo(short);
+
+void bar(int);
+void bar(short) _diagnose_if(1, "oh no", "error");
+
+void fooArg(int a) _diagnose_if(a, "oh no", "error"); // expected-note{{candidate disabled: oh no}}
+void fooArg(short); // expected-note{{candidate function}}
+
+void barArg(int);
+void barArg(short a) _diagnose_if(a, "oh no", "error");
+
+void runAll() {
+  foo(1); // expected-error{{oh no}}
+  bar(1);
+
+  fooArg(1); // expected-error{{call to unavailable function}}
+  barArg(1);
+
+  auto p = foo; // expected-error{{incompatible initializer of type ''}}
+}
+}
+
+namespace with_default_args {
+void foo(int a = 0) _diagnose_if(a, "oh no", "warning"); // expected-note 1{{from 'diagnose_if'}}
+void bar(int a = 1) _diagnose_if(a, "oh no", "warning"); // expected-note 2{{from 'diagnose_if'}}
+
+void runAll() {
+  foo();
+  foo(0);
+  foo(1); // 

[PATCH] D27424: Add the diagnose_if attribute to clang.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D27424#636496, @george.burgess.iv wrote:

> Do we have a page that details when we should/shouldn't use `auto`? I was 
> under the impression that it was preferred only in cases where the type's 
> spelled out (e.g. `cast`, ...). (To be clear, I'm happy to use it in 
> loops, too; I'd just like to know if we have guidelines about it. :) )


We do: 
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Btw, there's a question in there about late parsing that Phab won't let me 
delete, feel free to ignore it. ;-)




Comment at: include/clang/Basic/Attr.td:1584
+def DiagnoseIf : InheritableAttr {
+  let Spellings = [GNU<"diagnose_if">];
+  let Subjects = SubjectList<[Function]>;

george.burgess.iv wrote:
> aaron.ballman wrote:
> > I think this should also have a C++ spelling under the clang namespace.
> Agreed. Though, it looks like that would requires us teaching the parser how 
> to late-parse c++11 attrs if we wanted to do it properly. Given the size of 
> this patch, I think that can be done as a follow-up.
That seems reasonable to me.



Comment at: include/clang/Sema/Sema.h:2616
+  DiagnoseIfAttr *
+  checkArgDependentDiagnoseIf(FunctionDecl *Function, ArrayRef Args,
+  SmallVectorImpl ,

george.burgess.iv wrote:
> aaron.ballman wrote:
> > Can this (and the other new functions) accept the `FunctionDecl` as a 
> > `const FunctionDecl *` instead?
> Not easily, unless you'd prefer to see `const` here and a `const_cast` or two 
> in `checkArgDependentDiagnoseIf` over what we have now. I have no preference, 
> so if you would prefer to see that, I'm happy to do it.
> 
> If `ExprResult` and initialization routines like 
> `Sema::PerformCopyInitialization` were happy to take a `const Expr *`, this 
> would be a different story :)
That sounds like more churn than it's worth, to me. It'd be nice if we were 
better about const-correctness, but that's not this patch's problem to solve. 
;-)



Comment at: lib/AST/ExprConstant.cpp:10371
+assert(MD && "Don't provide `this` for non-methods.");
+assert(!MD->isStatic() && "Don't provide `this` for non-static methods.");
+#endif

I think the message means to say "Don't provide 'this' for static methods."



Comment at: lib/Parse/ParseDecl.cpp:371-372
 return;
   }
 
   // These may refer to the function arguments, but need to be parsed early to

In the last patch, this code was checking for `enable_if` or `diagnose_if`, but 
now only checks for `enable_if`. Do you not need this for `diagnose_if` because 
it's now late parsed?



Comment at: lib/Sema/SemaDeclAttr.cpp:949
+#ifndef NDEBUG
+if (auto *MD = dyn_cast(FD))
+  ClassType = MD->getParent();

`const auto *`


https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Do we have a page that details when we should/shouldn't use `auto`? I was under 
the impression that it was preferred only in cases where the type's spelled out 
(e.g. `cast`, ...). (To be clear, I'm happy to use it in loops, too; I'd 
just like to know if we have guidelines about it. :) )




Comment at: include/clang/Basic/Attr.td:1584
+def DiagnoseIf : InheritableAttr {
+  let Spellings = [GNU<"diagnose_if">];
+  let Subjects = SubjectList<[Function]>;

aaron.ballman wrote:
> I think this should also have a C++ spelling under the clang namespace.
Agreed. Though, it looks like that would requires us teaching the parser how to 
late-parse c++11 attrs if we wanted to do it properly. Given the size of this 
patch, I think that can be done as a follow-up.



Comment at: include/clang/Basic/DiagnosticCommonKinds.td:164
   InGroup;
+def ext_clang_diagnose_if : Extension<"'diagnose_if' is a clang extension">,
+InGroup;

aaron.ballman wrote:
> If we do add the clang namespaced spelling, we should have a test to ensure 
> that this diagnostic is not triggered when the attribute is spelled 
> `[[clang::diagnose_if(...)]]`.
Added a disabled test for this.



Comment at: include/clang/Sema/Sema.h:2616
+  DiagnoseIfAttr *
+  checkArgDependentDiagnoseIf(FunctionDecl *Function, ArrayRef Args,
+  SmallVectorImpl ,

aaron.ballman wrote:
> Can this (and the other new functions) accept the `FunctionDecl` as a `const 
> FunctionDecl *` instead?
Not easily, unless you'd prefer to see `const` here and a `const_cast` or two 
in `checkArgDependentDiagnoseIf` over what we have now. I have no preference, 
so if you would prefer to see that, I'm happy to do it.

If `ExprResult` and initialization routines like 
`Sema::PerformCopyInitialization` were happy to take a `const Expr *`, this 
would be a different story :)



Comment at: lib/Sema/SemaOverload.cpp:827
   destroyCandidates();
-  ConversionSequenceAllocator.Reset();
-  NumInlineSequences = 0;
+  // DiagnoseIfAttrs are just pointers, so we don't need to destroy them.
+  SlabAllocator.Reset();

aaron.ballman wrote:
> DiagnoseIfAttrs aren't the only thing allocated with the slab allocator 
> though, right?  Since this is being generalized, I wonder if asserting 
> somewhere that the slab allocator only deals with pointers would make sense?
It also allocates `ImplicitConversionSequence`s, but those are properly cleaned 
up by `destroyCandidates`. Added another `static_assert` to `slabAllocate` to 
hopefully catch errors if others try to use this.



Comment at: lib/Sema/SemaOverload.cpp:9009
+static bool isCandidateUnavailableDueToDiagnoseIf(const OverloadCandidate ) 
{
+  return OC.NumTriggeredDiagnoseIfs == 1 &&
+ OC.DiagnoseIfInfo.get()->isError();

aaron.ballman wrote:
> Can you run into a situation with stacked diagnose_if errors, so that there's 
> > 1?
Not at the moment (we stop evaluating diagnose_if attrs as soon as we find one 
that's an error), though you're right that this could be more robust.


https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 83188.
george.burgess.iv marked 18 inline comments as done.
george.burgess.iv added a comment.

Addressed all feedback + made `diagnose_if` late-parsed.


https://reviews.llvm.org/D27424

Files:
  include/clang/AST/Expr.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Initialization.h
  include/clang/Sema/Overload.h
  include/clang/Sema/Sema.h
  lib/AST/ExprConstant.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/Sema/diagnose_if.c
  test/SemaCXX/diagnose_if.cpp

Index: test/SemaCXX/diagnose_if.cpp
===
--- /dev/null
+++ test/SemaCXX/diagnose_if.cpp
@@ -0,0 +1,460 @@
+// RUN: %clang_cc1 %s -verify -fno-builtin -std=c++14
+
+#define _diagnose_if(...) __attribute__((diagnose_if(__VA_ARGS__)))
+
+namespace type_dependent {
+template 
+void neverok() _diagnose_if(!T(), "oh no", "error") {} // expected-note 4{{from 'diagnose_if'}}
+
+template 
+void alwaysok() _diagnose_if(T(), "oh no", "error") {}
+
+template 
+void alwayswarn() _diagnose_if(!T(), "oh no", "warning") {} // expected-note 4{{from 'diagnose_if'}}
+
+template 
+void neverwarn() _diagnose_if(T(), "oh no", "warning") {}
+
+void runAll() {
+  alwaysok();
+  alwaysok();
+
+  {
+void (*pok)() = alwaysok;
+pok = ;
+  }
+
+  neverok(); // expected-error{{oh no}}
+  neverok(); // expected-error{{oh no}}
+
+  {
+void (*pok)() = neverok; // expected-error{{oh no}}
+  }
+  {
+void (*pok)();
+pok = ; // expected-error{{oh no}}
+  }
+
+  alwayswarn(); // expected-warning{{oh no}}
+  alwayswarn(); // expected-warning{{oh no}}
+  {
+void (*pok)() = alwayswarn; // expected-warning{{oh no}}
+pok = ; // expected-warning{{oh no}}
+  }
+
+  neverwarn();
+  neverwarn();
+  {
+void (*pok)() = neverwarn;
+pok = ;
+  }
+}
+
+template 
+void errorIf(T a) _diagnose_if(T() != a, "oh no", "error") {} // expected-note {{candidate disabled: oh no}}
+
+template 
+void warnIf(T a) _diagnose_if(T() != a, "oh no", "warning") {} // expected-note {{from 'diagnose_if'}}
+
+void runIf() {
+  errorIf(0);
+  errorIf(1); // expected-error{{call to unavailable function}}
+
+  warnIf(0);
+  warnIf(1); // expected-warning{{oh no}}
+}
+}
+
+namespace value_dependent {
+template 
+void neverok() _diagnose_if(N == 0 || N != 0, "oh no", "error") {} // expected-note 4{{from 'diagnose_if'}}
+
+template 
+void alwaysok() _diagnose_if(N == 0 && N != 0, "oh no", "error") {}
+
+template 
+void alwayswarn() _diagnose_if(N == 0 || N != 0, "oh no", "warning") {} // expected-note 4{{from 'diagnose_if'}}
+
+template 
+void neverwarn() _diagnose_if(N == 0 && N != 0, "oh no", "warning") {}
+
+void runAll() {
+  alwaysok<0>();
+  alwaysok<1>();
+
+  {
+void (*pok)() = alwaysok<0>;
+pok = <0>;
+  }
+
+  neverok<0>(); // expected-error{{oh no}}
+  neverok<1>(); // expected-error{{oh no}}
+
+  {
+void (*pok)() = neverok<0>; // expected-error{{oh no}}
+  }
+  {
+void (*pok)();
+pok = <0>; // expected-error{{oh no}}
+  }
+
+  alwayswarn<0>(); // expected-warning{{oh no}}
+  alwayswarn<1>(); // expected-warning{{oh no}}
+  {
+void (*pok)() = alwayswarn<0>; // expected-warning{{oh no}}
+pok = <0>; // expected-warning{{oh no}}
+  }
+
+  neverwarn<0>();
+  neverwarn<1>();
+  {
+void (*pok)() = neverwarn<0>;
+pok = <0>;
+  }
+}
+
+template 
+void errorIf(int a) _diagnose_if(N != a, "oh no", "error") {} // expected-note {{candidate disabled: oh no}}
+
+template 
+void warnIf(int a) _diagnose_if(N != a, "oh no", "warning") {} // expected-note {{from 'diagnose_if'}}
+
+void runIf() {
+  errorIf<0>(0);
+  errorIf<0>(1); // expected-error{{call to unavailable function}}
+
+  warnIf<0>(0);
+  warnIf<0>(1); // expected-warning{{oh no}}
+}
+}
+
+namespace no_overload_interaction {
+void foo(int) _diagnose_if(1, "oh no", "error"); // expected-note{{from 'diagnose_if'}}
+void foo(short);
+
+void bar(int);
+void bar(short) _diagnose_if(1, "oh no", "error");
+
+void fooArg(int a) _diagnose_if(a, "oh no", "error"); // expected-note{{candidate disabled: oh no}}
+void fooArg(short); // expected-note{{candidate function}}
+
+void barArg(int);
+void barArg(short a) _diagnose_if(a, "oh no", "error");
+
+void runAll() {
+  foo(1); // expected-error{{oh no}}
+  bar(1);
+
+  fooArg(1); // expected-error{{call to unavailable function}}
+  barArg(1);
+
+  auto p = foo; // expected-error{{incompatible initializer of type ''}}
+}
+}
+
+namespace with_default_args {
+void foo(int a = 0) _diagnose_if(a, "oh no", "warning"); // expected-note 1{{from 'diagnose_if'}}
+void bar(int a = 1) _diagnose_if(a, "oh no", "warning"); // expected-note 2{{from 'diagnose_if'}}
+
+void runAll() {
+  

[PATCH] D27424: Add the diagnose_if attribute to clang.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I really like this attribute, thank you for working on it!




Comment at: include/clang/Basic/Attr.td:1584
+def DiagnoseIf : InheritableAttr {
+  let Spellings = [GNU<"diagnose_if">];
+  let Subjects = SubjectList<[Function]>;

I think this should also have a C++ spelling under the clang namespace.



Comment at: include/clang/Basic/AttrDocs.td:359
+  int val3 = abs(val);
+  int val4 = must_abs(val); // because run-time checks are not emitted for
+// diagnose_if attributes, this executes without

Capitalize the B in because.



Comment at: include/clang/Basic/DiagnosticCommonKinds.td:164
   InGroup;
+def ext_clang_diagnose_if : Extension<"'diagnose_if' is a clang extension">,
+InGroup;

If we do add the clang namespaced spelling, we should have a test to ensure 
that this diagnostic is not triggered when the attribute is spelled 
`[[clang::diagnose_if(...)]]`.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2145
+def err_diagnose_if_invalid_diagnostic_type : Error<
+  "invalid diagnostic type for diagnose_if. Try 'error' or 'warning'.">;
 def err_constexpr_body_no_return : Error<

Diagnostics don't have full stops, so I think a better way to write this is: 
`invalid diagnostic type for 'diagnose_if'; use \"error\" or \"warning\" 
instead`.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3361
 "pass_object_size attribute">;
-def note_ovl_candidate_disabled_by_enable_if_attr : Note<
+def err_diagnose_if_succeeded: Error<"%0">;
+def warn_diagnose_if_succeeded : Warning<"%0">, InGroup;

Space before the colon.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4380
 InGroup;
+def note_from_diagnose_if : Note<"from diagnose_if attribute on %0:">;
 def warn_property_method_deprecated :

Please single quote the use of diagnose_if.



Comment at: include/clang/Sema/Overload.h:664
+
+/// Basically an TinyPtrVector that doesn't own the
+/// vector: If NumTriggeredDiagnoseIfs is 0 or 1, this is a DiagnoseIfAttr

an -> a



Comment at: include/clang/Sema/Sema.h:2616
+  DiagnoseIfAttr *
+  checkArgDependentDiagnoseIf(FunctionDecl *Function, ArrayRef Args,
+  SmallVectorImpl ,

Can this (and the other new functions) accept the `FunctionDecl` as a `const 
FunctionDecl *` instead?



Comment at: lib/Sema/SemaDeclAttr.cpp:857
+public:
+  ParmVarDeclChecker(FunctionDecl *FD) {
+Parms.insert(FD->param_begin(), FD->param_end());

This should accept `FD` as a const pointer.



Comment at: lib/Sema/SemaExpr.cpp:368
+
+if (DiagnoseIfAttr *A =
+checkArgIndependentDiagnoseIf(FD, DiagnoseIfWarnings)) {

`const DiagnoseIfAttr *`?



Comment at: lib/Sema/SemaExpr.cpp:388
+
+  for (DiagnoseIfAttr *W : DiagnoseIfWarnings)
+emitDiagnoseIfDiagnostic(Loc, W);

`const auto *`?



Comment at: lib/Sema/SemaExpr.cpp:5186
+
+  for (DiagnoseIfAttr *Attr : Nonfatal)
+S.emitDiagnoseIfDiagnostic(Fn->getLocStart(), Attr);

`const auto *`?



Comment at: lib/Sema/SemaOverload.cpp:827
   destroyCandidates();
-  ConversionSequenceAllocator.Reset();
-  NumInlineSequences = 0;
+  // DiagnoseIfAttrs are just pointers, so we don't need to destroy them.
+  SlabAllocator.Reset();

DiagnoseIfAttrs aren't the only thing allocated with the slab allocator though, 
right?  Since this is being generalized, I wonder if asserting somewhere that 
the slab allocator only deals with pointers would make sense?



Comment at: lib/Sema/SemaOverload.cpp:6151
+  bool MissingImplicitThis) {
+  auto EnableIfAttrs = getOrderedEnableIfAttrs(Function);
+  if (EnableIfAttrs.empty())

Please spell out the type instead of using `auto`, since the type isn't spelled 
in the initialization.



Comment at: lib/Sema/SemaOverload.cpp:6178-6179
+  SmallVectorImpl ) 
{
+  for (Attr *A : Function->attrs())
+if (auto *DIA = dyn_cast(A))
+  if (ArgDependent == DIA->getArgDependent()) {

Instead of doing this, you can use `specific_attrs()`



Comment at: lib/Sema/SemaOverload.cpp:9009
+static bool isCandidateUnavailableDueToDiagnoseIf(const OverloadCandidate ) 
{
+  return OC.NumTriggeredDiagnoseIfs == 1 &&
+ OC.DiagnoseIfInfo.get()->isError();

Can you run into a situation with stacked diagnose_if errors, so that there's > 
1?



Comment at: lib/Sema/SemaOverload.cpp:9101
 
+  for 

[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 81259.
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.

Addressed alignment comment. Thanks! (Late parsing is still to-be-done)


https://reviews.llvm.org/D27424

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Overload.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/Sema/diagnose_if.c
  test/SemaCXX/diagnose_if.cpp

Index: test/SemaCXX/diagnose_if.cpp
===
--- /dev/null
+++ test/SemaCXX/diagnose_if.cpp
@@ -0,0 +1,352 @@
+// RUN: %clang_cc1 %s -verify -fno-builtin -std=c++11
+
+#define _diagnose_if(...) __attribute__((diagnose_if(__VA_ARGS__)))
+
+namespace type_dependent {
+template 
+void neverok() _diagnose_if(!T(), "oh no", "error") {} // expected-note 4{{from diagnose_if}}
+
+template 
+void alwaysok() _diagnose_if(T(), "oh no", "error") {}
+
+template 
+void alwayswarn() _diagnose_if(!T(), "oh no", "warning") {} // expected-note 4{{from diagnose_if}}
+
+template 
+void neverwarn() _diagnose_if(T(), "oh no", "warning") {}
+
+void runAll() {
+  alwaysok();
+  alwaysok();
+
+  {
+void (*pok)() = alwaysok;
+pok = ;
+  }
+
+  neverok(); // expected-error{{oh no}}
+  neverok(); // expected-error{{oh no}}
+
+  {
+void (*pok)() = neverok; // expected-error{{oh no}}
+  }
+  {
+void (*pok)();
+pok = ; // expected-error{{oh no}}
+  }
+
+  alwayswarn(); // expected-warning{{oh no}}
+  alwayswarn(); // expected-warning{{oh no}}
+  {
+void (*pok)() = alwayswarn; // expected-warning{{oh no}}
+pok = ; // expected-warning{{oh no}}
+  }
+
+  neverwarn();
+  neverwarn();
+  {
+void (*pok)() = neverwarn;
+pok = ;
+  }
+}
+
+template 
+void errorIf(T a) _diagnose_if(T() != a, "oh no", "error") {} // expected-note {{candidate disabled: oh no}}
+
+template 
+void warnIf(T a) _diagnose_if(T() != a, "oh no", "warning") {} // expected-note {{from diagnose_if}}
+
+void runIf() {
+  errorIf(0);
+  errorIf(1); // expected-error{{call to unavailable function}}
+
+  warnIf(0);
+  warnIf(1); // expected-warning{{oh no}}
+}
+}
+
+namespace value_dependent {
+template 
+void neverok() _diagnose_if(N == 0 || N != 0, "oh no", "error") {} // expected-note 4{{from diagnose_if}}
+
+template 
+void alwaysok() _diagnose_if(N == 0 && N != 0, "oh no", "error") {}
+
+template 
+void alwayswarn() _diagnose_if(N == 0 || N != 0, "oh no", "warning") {} // expected-note 4{{from diagnose_if}}
+
+template 
+void neverwarn() _diagnose_if(N == 0 && N != 0, "oh no", "warning") {}
+
+void runAll() {
+  alwaysok<0>();
+  alwaysok<1>();
+
+  {
+void (*pok)() = alwaysok<0>;
+pok = <0>;
+  }
+
+  neverok<0>(); // expected-error{{oh no}}
+  neverok<1>(); // expected-error{{oh no}}
+
+  {
+void (*pok)() = neverok<0>; // expected-error{{oh no}}
+  }
+  {
+void (*pok)();
+pok = <0>; // expected-error{{oh no}}
+  }
+
+  alwayswarn<0>(); // expected-warning{{oh no}}
+  alwayswarn<1>(); // expected-warning{{oh no}}
+  {
+void (*pok)() = alwayswarn<0>; // expected-warning{{oh no}}
+pok = <0>; // expected-warning{{oh no}}
+  }
+
+  neverwarn<0>();
+  neverwarn<1>();
+  {
+void (*pok)() = neverwarn<0>;
+pok = <0>;
+  }
+}
+
+template 
+void errorIf(int a) _diagnose_if(N != a, "oh no", "error") {} // expected-note {{candidate disabled: oh no}}
+
+template 
+void warnIf(int a) _diagnose_if(N != a, "oh no", "warning") {} // expected-note {{from diagnose_if}}
+
+void runIf() {
+  errorIf<0>(0);
+  errorIf<0>(1); // expected-error{{call to unavailable function}}
+
+  warnIf<0>(0);
+  warnIf<0>(1); // expected-warning{{oh no}}
+}
+}
+
+namespace no_overload_interaction {
+void foo(int) _diagnose_if(1, "oh no", "error"); // expected-note{{from diagnose_if}}
+void foo(short);
+
+void bar(int);
+void bar(short) _diagnose_if(1, "oh no", "error");
+
+void fooArg(int a) _diagnose_if(a, "oh no", "error"); // expected-note{{candidate disabled: oh no}}
+void fooArg(short); // expected-note{{candidate function}}
+
+void barArg(int);
+void barArg(short a) _diagnose_if(a, "oh no", "error");
+
+void runAll() {
+  foo(1); // expected-error{{oh no}}
+  bar(1);
+
+  fooArg(1); // expected-error{{call to unavailable function}}
+  barArg(1);
+
+  auto p = foo; // expected-error{{incompatible initializer of type ''}}
+}
+}
+
+// We have some inline space craziness going on. This is meant to go over that
+// in hopes of hitting some edge cases. At the time of writing, our inline
+// buffer can support 16 DiagnoseIfAttr*s. If a function only has one attr, no
+// allocation is done.
+namespace overloading_multiple_coverage {
+#define _diagnose_thirteen_times \
+  

[PATCH] D27424: Add the diagnose_if attribute to clang.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/Sema/Overload.h:754
+unsigned NumInlineBytesUsed;
+llvm::AlignedCharArray
 InlineSpace;

I guess you could use "void *" instead of ImplicitConversionSequence for the 
alignment to make it clear that this has the alignment of a pointer type (or 
the larger of alignof(ImplicitConversionSequence) and alignof(DiagnoseIfAttr 
*))?


https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Glad you like it!

> The second thing that would be amazing if this attribute was late parsed so 
> it the ability to reference this when applied to member functions

That seems like a good idea; I'll look into what that would take. (In the 
meantime, reviews would be appreciated. :) )

> If the condition could be evaluated as-if it were part of the function body 
> (during constexpr evaluation) that would be incredibly powerful. Especially 
> within the STL where everything is constexpr.

Honestly, if we could take this attribute and turn it into a full-fledged 
precondition attribute (that optionally acts as if it's a part of the function, 
optionally emits dynamic checks, ...), I'd be thrilled. If the community thinks 
that'd be a good idea, I'm happy to poke at it + incrementally build off of 
this in order to make that*. That said, I think all of that is going to require 
a fair bit more code and design thought to get right, and `diagnose_if` is 
useful enough as-is to stand on its own IMO.

IOW, we need to start somewhere, and this is a minimial-ish feature set that I 
think would be useful.

∗ - We can do this in a non-breaking way by either introducing an independent 
`precondition` attribute that heavily piggybacks off of `diagnose_if`, or we 
can add diagnostic modes + flags to `diagnose_if` (e.g. can have `warning`, 
`error`, `static-precondition`, `dynamic-precondition`, ...).


https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

Awesome, this is exactly what I was hoping for! Thanks for working on this!

I'm a bit surprised that the attribute fails to evaluate the condition as a 
constant expression during the evaluation of constexpr functions. 
Ex:

  #define DIAG(...) __attribute__((diagnose_if(__VA_ARGS__, "message", 
"error")))
  constexpr bool foo(int x) DIAG(x == 0) // fails to evaluate this
  { 
// assert(x != 0); <-- ironically a simple assert would catch this
return x == 0;
  }  
  constexpr bool bar(int x) { return foo(x); }
  static_assert(bar(0), "");

Clearly the compiler can evaluate `x == 0` as a constant expression, but the 
attribute fails to see this. If the condition could be evaluated as-if it were 
part of the function body (during constexpr evaluation) that would be 
incredibly powerful. Especially within the STL where everything is `constexpr`.

The second thing that would be amazing if this attribute was late parsed so it  
the ability to reference `this` when applied to member functions.
For example:

  struct Foo {
bool is_good;
constexpr bool good() const { return is_good; }
constexpr bool foo() const _diagnose_if(!good(), "message", "error")
{return true; }
  };
  constexpr Foo f{false};
  static_assert(f.foo(), "");

Thanks again for working on this! I'm very excited to get to use it!


https://reviews.llvm.org/D27424



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

[PATCH] D27424: Add the diagnose_if attribute to clang.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: rsmith, aaron.ballman.
george.burgess.iv added subscribers: EricWF, cfe-commits.

This patch adds the `diagnose_if` attribute to clang.

`diagnose_if` is meant to be a less powerful version of `enable_if`: it doesn't 
interact with overload resolution at all, and if the condition in the 
`diagnose_if` attribute can't be evaluated, compilation proceeds as though said 
`diagnose_if` attribute was not present. It can be used to emit either errors 
or warnings, like so:

  void foo(int a) __attribute__((diagnose_if(a > 10, "a is too large!", 
"error")));
  void bar(int a) __attribute__((diagnose_if(a > 10, "this takes a long time if 
a is larger than 10.", "warning")));
  void baz(int a) __attribute__((diagnose_if(1, "don't use me anymore", 
"warning")));
  
  void run(int a) {
foo(10);
foo(11); // error: a is too large
foo(a);
bar(10);
bar(11); // warning: this takes a long time if a is larger than 10.
bar(a);
  
void (*b)(int) = baz; // warning: don't use me anymore.
  }

Under the hood, we have two kinds of `diagnose_if` attributes: ones that are 
dependent on the value of arguments (which are handled a lot like `enable_if`), 
and ones that aren't (which are treated like `unavailable`/`deprecated` 
attributes). This split made this attribute *substantially* less complex to 
implement, since it lets us reuse existing code


https://reviews.llvm.org/D27424

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Overload.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  test/Sema/diagnose_if.c
  test/SemaCXX/diagnose_if.cpp

Index: test/SemaCXX/diagnose_if.cpp
===
--- /dev/null
+++ test/SemaCXX/diagnose_if.cpp
@@ -0,0 +1,352 @@
+// RUN: %clang_cc1 %s -verify -fno-builtin -std=c++11
+
+#define _diagnose_if(...) __attribute__((diagnose_if(__VA_ARGS__)))
+
+namespace type_dependent {
+template 
+void neverok() _diagnose_if(!T(), "oh no", "error") {} // expected-note 4{{from diagnose_if}}
+
+template 
+void alwaysok() _diagnose_if(T(), "oh no", "error") {}
+
+template 
+void alwayswarn() _diagnose_if(!T(), "oh no", "warning") {} // expected-note 4{{from diagnose_if}}
+
+template 
+void neverwarn() _diagnose_if(T(), "oh no", "warning") {}
+
+void runAll() {
+  alwaysok();
+  alwaysok();
+
+  {
+void (*pok)() = alwaysok;
+pok = ;
+  }
+
+  neverok(); // expected-error{{oh no}}
+  neverok(); // expected-error{{oh no}}
+
+  {
+void (*pok)() = neverok; // expected-error{{oh no}}
+  }
+  {
+void (*pok)();
+pok = ; // expected-error{{oh no}}
+  }
+
+  alwayswarn(); // expected-warning{{oh no}}
+  alwayswarn(); // expected-warning{{oh no}}
+  {
+void (*pok)() = alwayswarn; // expected-warning{{oh no}}
+pok = ; // expected-warning{{oh no}}
+  }
+
+  neverwarn();
+  neverwarn();
+  {
+void (*pok)() = neverwarn;
+pok = ;
+  }
+}
+
+template 
+void errorIf(T a) _diagnose_if(T() != a, "oh no", "error") {} // expected-note {{candidate disabled: oh no}}
+
+template 
+void warnIf(T a) _diagnose_if(T() != a, "oh no", "warning") {} // expected-note {{from diagnose_if}}
+
+void runIf() {
+  errorIf(0);
+  errorIf(1); // expected-error{{call to unavailable function}}
+
+  warnIf(0);
+  warnIf(1); // expected-warning{{oh no}}
+}
+}
+
+namespace value_dependent {
+template 
+void neverok() _diagnose_if(N == 0 || N != 0, "oh no", "error") {} // expected-note 4{{from diagnose_if}}
+
+template 
+void alwaysok() _diagnose_if(N == 0 && N != 0, "oh no", "error") {}
+
+template 
+void alwayswarn() _diagnose_if(N == 0 || N != 0, "oh no", "warning") {} // expected-note 4{{from diagnose_if}}
+
+template 
+void neverwarn() _diagnose_if(N == 0 && N != 0, "oh no", "warning") {}
+
+void runAll() {
+  alwaysok<0>();
+  alwaysok<1>();
+
+  {
+void (*pok)() = alwaysok<0>;
+pok = <0>;
+  }
+
+  neverok<0>(); // expected-error{{oh no}}
+  neverok<1>(); // expected-error{{oh no}}
+
+  {
+void (*pok)() = neverok<0>; // expected-error{{oh no}}
+  }
+  {
+void (*pok)();
+pok = <0>; // expected-error{{oh no}}
+  }
+
+  alwayswarn<0>(); // expected-warning{{oh no}}
+  alwayswarn<1>(); // expected-warning{{oh no}}
+  {
+void (*pok)() = alwayswarn<0>; // expected-warning{{oh no}}
+pok = <0>; // expected-warning{{oh no}}
+  }
+
+  neverwarn<0>();
+  neverwarn<1>();
+  {
+void (*pok)() = neverwarn<0>;
+pok = <0>;
+  }
+}
+
+template 
+void errorIf(int a) _diagnose_if(N != a, "oh no", "error") {} // expected-note {{candidate disabled: oh no}}
+
+template 
+void warnIf(int a) _diagnose_if(N != a, "oh no", "warning") {} //