[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<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. :) )



================
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<GccCompat>;
+def ext_clang_diagnose_if : Extension<"'diagnose_if' is a clang extension">,
+                            InGroup<GccCompat>;
----------------
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<Expr *> Args,
+                              SmallVectorImpl<DiagnoseIfAttr *> &Nonfatal,
----------------
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 &OC) {
+  return OC.NumTriggeredDiagnoseIfs == 1 &&
+         OC.DiagnoseIfInfo.get<DiagnoseIfAttr *>()->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