[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<Foo>`, ...). (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<Expr *> Args,
+                              SmallVectorImpl<DiagnoseIfAttr *> &Nonfatal,
----------------
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<CXXMethodDecl>(FD))
+      ClassType = MD->getParent();
----------------
`const auto *`


https://reviews.llvm.org/D27424