r303712 - Enhance the 'diagnose_if' attribute so that we can apply it for ObjC methods and properties as well
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Wed May 24 10:58:28 PDT 2017
On Wed, May 24, 2017 at 1:37 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>
> On May 24, 2017, at 10:12 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Wed, May 24, 2017 at 1:05 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>
> wrote:
>
>
> On May 24, 2017, at 8:59 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Tue, May 23, 2017 at 8:46 PM, Argyrios Kyrtzidis via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>
> Author: akirtzidis
> Date: Tue May 23 19:46:27 2017
> New Revision: 303712
>
> URL: http://llvm.org/viewvc/llvm-project?rev=303712&view=rev
> Log:
> Enhance the 'diagnose_if' attribute so that we can apply it for ObjC methods
> and properties as well
>
> This is an initial commit to allow using it with constant expressions, a
> follow-up commit will enable full support for it in ObjC methods.
>
> Added:
> cfe/trunk/test/SemaObjC/diagnose_if.m
> Modified:
> cfe/trunk/include/clang/Basic/Attr.td
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/include/clang/Sema/AttributeList.h
> cfe/trunk/include/clang/Sema/Sema.h
> cfe/trunk/lib/Lex/PPMacroExpansion.cpp
> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> cfe/trunk/lib/Sema/SemaExpr.cpp
> cfe/trunk/lib/Sema/SemaOverload.cpp
> cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
>
> Modified: cfe/trunk/include/clang/Basic/Attr.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/Attr.td (original)
> +++ cfe/trunk/include/clang/Basic/Attr.td Tue May 23 19:46:27 2017
> @@ -149,6 +149,9 @@ class ExprArgument<string name, bit opt
> class FunctionArgument<string name, bit opt = 0, bit fake = 0> :
> Argument<name,
>
> opt,
>
> fake>;
> +class NamedArgument<string name, bit opt = 0, bit fake = 0> :
> Argument<name,
> +
> opt,
> +
> fake>;
> class TypeArgument<string name, bit opt = 0> : Argument<name, opt>;
> class UnsignedArgument<string name, bit opt = 0> : Argument<name, opt>;
> class VariadicUnsignedArgument<string name> : Argument<name, 1>;
> @@ -1819,14 +1822,14 @@ def Unavailable : InheritableAttr {
>
> def DiagnoseIf : InheritableAttr {
> let Spellings = [GNU<"diagnose_if">];
> - let Subjects = SubjectList<[Function]>;
> + let Subjects = SubjectList<[Function, ObjCMethod, ObjCProperty]>;
> let Args = [ExprArgument<"Cond">, StringArgument<"Message">,
> EnumArgument<"DiagnosticType",
> "DiagnosticType",
> ["error", "warning"],
> ["DT_Error", "DT_Warning"]>,
> BoolArgument<"ArgDependent", 0, /*fake*/ 1>,
> - FunctionArgument<"Parent", 0, /*fake*/ 1>];
> + NamedArgument<"Parent", 0, /*fake*/ 1>];
> let DuplicatesAllowedWhileMerging = 1;
> let LateParsed = 1;
> let AdditionalMembers = [{
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue May 23 19:46:27
> 2017
> @@ -2771,6 +2771,7 @@ def warn_attribute_wrong_decl_type : War
> "|types and namespaces"
> "|Objective-C interfaces"
> "|methods and properties"
> + "|functions, methods and properties"
>
>
> functions, methods, and properties (inserting the Oxford comma).
>
>
> Ok.
>
>
> "|struct or union"
> "|struct, union or class"
> "|types"
>
> Modified: cfe/trunk/include/clang/Sema/AttributeList.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/AttributeList.h?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/AttributeList.h (original)
> +++ cfe/trunk/include/clang/Sema/AttributeList.h Tue May 23 19:46:27 2017
> @@ -915,6 +915,7 @@ enum AttributeDeclKind {
> ExpectedTypeOrNamespace,
> ExpectedObjectiveCInterface,
> ExpectedMethodOrProperty,
> + ExpectedFunctionOrMethodOrProperty,
> ExpectedStructOrUnion,
> ExpectedStructOrUnionOrClass,
> ExpectedType,
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Tue May 23 19:46:27 2017
> @@ -2727,7 +2727,7 @@ public:
> /// of a function.
> ///
> /// Returns true if any errors were emitted.
> - bool diagnoseArgIndependentDiagnoseIfAttrs(const FunctionDecl *Function,
> + bool diagnoseArgIndependentDiagnoseIfAttrs(const NamedDecl *ND,
> SourceLocation Loc);
>
> /// Returns whether the given function's address can be taken or not,
>
> Modified: cfe/trunk/lib/Lex/PPMacroExpansion.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPMacroExpansion.cpp?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Lex/PPMacroExpansion.cpp (original)
> +++ cfe/trunk/lib/Lex/PPMacroExpansion.cpp Tue May 23 19:46:27 2017
> @@ -1166,6 +1166,7 @@ static bool HasFeature(const Preprocesso
> .Case("objc_generics", LangOpts.ObjC2)
> .Case("objc_generics_variance", LangOpts.ObjC2)
> .Case("objc_class_property", LangOpts.ObjC2)
> + .Case("objc_diagnose_if_attr", LangOpts.ObjC2)
> // C11 features
> .Case("c_alignas", LangOpts.C11)
> .Case("c_alignof", LangOpts.C11)
>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Tue May 23 19:46:27 2017
> @@ -949,7 +949,7 @@ static bool checkFunctionConditionAttr(S
> Msg = "<no message provided>";
>
> SmallVector<PartialDiagnosticAt, 8> Diags;
> - if (!Cond->isValueDependent() &&
> + if (isa<FunctionDecl>(D) && !Cond->isValueDependent() &&
> !Expr::isPotentialConstantExprUnevaluated(Cond, cast<FunctionDecl>(D),
> Diags)) {
> S.Diag(Attr.getLoc(), diag::err_attr_cond_never_constant_expr)
> @@ -1037,10 +1037,11 @@ static void handleDiagnoseIfAttr(Sema &S
> return;
> }
>
> - auto *FD = cast<FunctionDecl>(D);
> - bool ArgDependent = ArgumentDependenceChecker(FD).referencesArgs(Cond);
> + bool ArgDependent = false;
> + if (auto *FD = dyn_cast<FunctionDecl>(D))
>
>
> Please use const auto *.
>
> + ArgDependent = ArgumentDependenceChecker(FD).referencesArgs(Cond);
> D->addAttr(::new (S.Context) DiagnoseIfAttr(
> - Attr.getRange(), S.Context, Cond, Msg, DiagType, ArgDependent, FD,
> + Attr.getRange(), S.Context, Cond, Msg, DiagType, ArgDependent,
> cast<NamedDecl>(D),
> Attr.getAttributeSpellingListIndex()));
> }
>
>
> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Tue May 23 19:46:27 2017
> @@ -366,8 +366,19 @@ bool Sema::DiagnoseUseOfDecl(NamedDecl *
>
> if (getLangOpts().CUDA && !CheckCUDACall(Loc, FD))
> return true;
> + }
>
> - if (diagnoseArgIndependentDiagnoseIfAttrs(FD, Loc))
> + auto getReferencedObjCProp = [](const NamedDecl *D) ->
> + const ObjCPropertyDecl * {
> + if (auto *MD = dyn_cast<ObjCMethodDecl>(D))
>
>
> Please use const auto *.
>
>
> What is the rationale for this, the casted object is ‘const’ already so it’s
> unnecessary, and I don’t see a mention in coding conventions that we should
> be using ‘const’ explicitly along with auto.
>
>
> That is our usual advice -- why should the reader have to guess
> whether the pointee type is const or not? It is in the coding
> standard, but could stand to be called out more explicitly. See the
> last example in
> http://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto.
>
>
> The last example is not clear whether the original pointer was also const so
> it added ‘const’ redundantly or it added ‘const’ specifically for the
> duration of the ‘for’ loop.
Fair.
> Which is why I have a slight preference to omit it, it’s not that I want it
> to be ‘const’ specifically for the ‘if’ block, I’m passing it along, so if I
> want it ‘const’ it should be ‘const’ at the ‘source’.
I disagree with that in this context. The uses of that variable in the
block are all const, so adding const is codifying your expectations
with the type system. You don't expect that use of the variable to be
mutated, so it should be explicitly spelled out. Having "silent"
qualifiers (where the type really is const but there's no mention of
it in the source code without following an arbitrary chain of inferred
types) is a frustrating thing to come across when maintaining code.
> In any case, I don’t feel strongly about it, I’ll make the changes, but if
> this is the consensus it should be explicit in the conventions document.
Thank you for making the change. I think the conventions document
could stand to be updated.
~Aaron
>
>
> ~Aaron
>
>
> + return MD->findPropertyDecl();
> + return nullptr;
> + };
> + if (auto *ObjCPDecl = getReferencedObjCProp(D)) {
>
>
> Please do not use auto here (the type is not spelled out in the
> initialization). Also, please mark it explicitly as being a const
> pointer.
>
>
> Ok.
>
>
> + if (diagnoseArgIndependentDiagnoseIfAttrs(ObjCPDecl, Loc))
> + return true;
> + } else {
> + if (diagnoseArgIndependentDiagnoseIfAttrs(D, Loc))
>
>
> Please bump the if up to be part of the else clause.
>
>
> Ok.
>
>
> ~Aaron
>
> return true;
> }
>
>
> Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaOverload.cpp Tue May 23 19:46:27 2017
> @@ -6242,11 +6242,11 @@ EnableIfAttr *Sema::CheckEnableIf(Functi
> }
>
> template <typename CheckFn>
> -static bool diagnoseDiagnoseIfAttrsWith(Sema &S, const FunctionDecl *FD,
> +static bool diagnoseDiagnoseIfAttrsWith(Sema &S, const NamedDecl *ND,
> bool ArgDependent, SourceLocation
> Loc,
> CheckFn &&IsSuccessful) {
> SmallVector<const DiagnoseIfAttr *, 8> Attrs;
> - for (const auto *DIA : FD->specific_attrs<DiagnoseIfAttr>()) {
> + for (const auto *DIA : ND->specific_attrs<DiagnoseIfAttr>()) {
> if (ArgDependent == DIA->getArgDependent())
> Attrs.push_back(DIA);
> }
> @@ -6293,16 +6293,16 @@ bool Sema::diagnoseArgDependentDiagnoseI
> // EvaluateWithSubstitution only cares about the position of each
> // argument in the arg list, not the ParmVarDecl* it maps to.
> if (!DIA->getCond()->EvaluateWithSubstitution(
> - Result, Context, DIA->getParent(), Args, ThisArg))
> + Result, Context, cast<FunctionDecl>(DIA->getParent()),
> Args, ThisArg))
> return false;
> return Result.isInt() && Result.getInt().getBoolValue();
> });
> }
>
> -bool Sema::diagnoseArgIndependentDiagnoseIfAttrs(const FunctionDecl
> *Function,
> +bool Sema::diagnoseArgIndependentDiagnoseIfAttrs(const NamedDecl *ND,
> SourceLocation Loc) {
> return diagnoseDiagnoseIfAttrsWith(
> - *this, Function, /*ArgDependent=*/false, Loc,
> + *this, ND, /*ArgDependent=*/false, Loc,
> [&](const DiagnoseIfAttr *DIA) {
> bool Result;
> return DIA->getCond()->EvaluateAsBooleanCondition(Result, Context) &&
>
> Added: cfe/trunk/test/SemaObjC/diagnose_if.m
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/diagnose_if.m?rev=303712&view=auto
> ==============================================================================
> --- cfe/trunk/test/SemaObjC/diagnose_if.m (added)
> +++ cfe/trunk/test/SemaObjC/diagnose_if.m Tue May 23 19:46:27 2017
> @@ -0,0 +1,16 @@
> +// RUN: %clang_cc1 %s -verify -fno-builtin
> +
> +_Static_assert(__has_feature(objc_diagnose_if_attr), "feature check
> failed?");
> +
> +#define _diagnose_if(...) __attribute__((diagnose_if(__VA_ARGS__)))
> +
> + at interface I
> +-(void)meth _diagnose_if(1, "don't use this", "warning"); // expected-note
> 1{{from 'diagnose_if'}}
> + at property (assign) id prop _diagnose_if(1, "don't use this", "warning"); //
> expected-note 2{{from 'diagnose_if'}}
> + at end
> +
> +void test(I *i) {
> + [i meth]; // expected-warning {{don't use this}}
> + id o1 = i.prop; // expected-warning {{don't use this}}
> + id o2 = [i prop]; // expected-warning {{don't use this}}
> +}
>
> Modified: cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp?rev=303712&r1=303711&r2=303712&view=diff
> ==============================================================================
> --- cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp (original)
> +++ cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp Tue May 23 19:46:27 2017
> @@ -312,7 +312,7 @@ namespace {
> }
>
> void writeDump(raw_ostream &OS) const override {
> - if (type == "FunctionDecl *") {
> + if (type == "FunctionDecl *" || type == "NamedDecl *") {
> OS << " OS << \" \";\n";
> OS << " dumpBareDeclRef(SA->get" << getUpperName() << "());\n";
> } else if (type == "IdentifierInfo *") {
> @@ -1181,6 +1181,8 @@ createArgument(const Record &Arg, String
> Ptr = llvm::make_unique<ExprArgument>(Arg, Attr);
> else if (ArgName == "FunctionArgument")
> Ptr = llvm::make_unique<SimpleArgument>(Arg, Attr, "FunctionDecl *");
> + else if (ArgName == "NamedArgument")
> + Ptr = llvm::make_unique<SimpleArgument>(Arg, Attr, "NamedDecl *");
> else if (ArgName == "IdentifierArgument")
> Ptr = llvm::make_unique<SimpleArgument>(Arg, Attr, "IdentifierInfo *");
> else if (ArgName == "DefaultBoolArgument")
> @@ -3103,6 +3105,8 @@ static std::string CalculateDiagnostic(c
> " : ExpectedVariableOrFunction))";
>
> case ObjCMethod | ObjCProp: return "ExpectedMethodOrProperty";
> + case Func | ObjCMethod | ObjCProp:
> + return "ExpectedFunctionOrMethodOrProperty";
> case ObjCProtocol | ObjCInterface:
> return "ExpectedObjectiveCInterfaceOrProtocol";
> case Field | Var: return "ExpectedFieldOrGlobalVar";
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
More information about the cfe-commits
mailing list