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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
