[PATCH] consumable attribute default state

Aaron Ballman aaron.ballman at gmail.com
Wed Sep 4 12:21:05 PDT 2013


Minor nits as well:

> Index: include/clang/Analysis/Analyses/Consumed.h
> ===================================================================
> --- include/clang/Analysis/Analyses/Consumed.h
> +++ include/clang/Analysis/Analyses/Consumed.h
> @@ -190,6 +190,8 @@
>
>      ConsumedState ExpectedReturnState;
>
> +    void determineExpectedReturnState(AnalysisDeclContext &AC,
> +                                      const FunctionDecl *D);
>      bool hasConsumableAttributes(const CXXRecordDecl *RD);
>      bool splitState(const CFGBlock *CurrBlock,
>                      const ConsumedStmtVisitor &Visitor);
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -931,6 +931,9 @@
>  def Consumable : InheritableAttr {
>    let Spellings = [GNU<"consumable">];
>    let Subjects = [CXXRecord];
> +  let Args = [EnumArgument<"DefaultState", "ConsumedState",
> +                           ["unknown", "consumed", "unconsumed"],
> +                           ["Unknown", "Consumed", "Unconsumed"]>];
>  }
>
>  def CallableWhenUnconsumed : InheritableAttr {
> Index: lib/Analysis/Consumed.cpp
> ===================================================================
> --- lib/Analysis/Consumed.cpp
> +++ lib/Analysis/Consumed.cpp
> @@ -89,9 +89,25 @@
>    return FunDecl->hasAttr<TestsUnconsumedAttr>();
>  }
>
> +static ConsumedState mapConsumableAttrState(const QualType QT) {
> +  assert(isConsumableType(QT));
> +
> +  const ConsumableAttr *CAttr =
> +    QT->getAsCXXRecordDecl()->getAttr<ConsumableAttr>();
> +
> +  switch (CAttr->getDefaultState()) {
> +  case ConsumableAttr::Unknown:
> +    return CS_Unknown;
> +  case ConsumableAttr::Unconsumed:
> +    return CS_Unconsumed;
> +  case ConsumableAttr::Consumed:
> +    return CS_Consumed;
> +  }
> +  llvm_unreachable("invalid enum");
> +}
> +
>  static ConsumedState
>  mapReturnTypestateAttrState(const ReturnTypestateAttr *RTSAttr) {
> -
>    switch (RTSAttr->getState()) {
>    case ReturnTypestateAttr::Unknown:
>      return CS_Unknown;
> @@ -402,7 +418,7 @@
>        ReturnState = mapReturnTypestateAttrState(
>          Fun->getAttr<ReturnTypestateAttr>());
>      else
> -      ReturnState = CS_Unknown;
> +      ReturnState = mapConsumableAttrState(ReturnType);
>
>      PropagationMap.insert(PairType(Call,
>        PropagationInfo(ReturnState)));
> @@ -709,8 +725,22 @@
>
>
>  void ConsumedStmtVisitor::VisitParmVarDecl(const ParmVarDecl *Param) {
> -  if (isConsumableType(Param->getType()))
> -    StateMap->setState(Param, consumed::CS_Unknown);
> +  QualType ParamType = Param->getType();
> +  ConsumedState ParamState = consumed::CS_None;
> +
> +  if (!(ParamType->isPointerType() || ParamType->isReferenceType()) &&
> +      isConsumableType(ParamType)) {
> +
> +    ParamState = mapConsumableAttrState(ParamType);
> +
> +  } else if (ParamType->isReferenceType() &&
> +             isConsumableType(ParamType->getPointeeType())) {
> +
> +    ParamState = consumed::CS_Unknown;
> +  }

The if and else if shouldn't have the curly braces since it's just a
single line.  Also, the extra newline isn't required.

> +
> +  if (ParamState)
> +    StateMap->setState(Param, ParamState);
>  }
>
>  void ConsumedStmtVisitor::VisitReturnStmt(const ReturnStmt *Ret) {
> @@ -952,6 +982,42 @@
>    Map.erase(Var);
>  }
>
> +void ConsumedAnalyzer::determineExpectedReturnState(AnalysisDeclContext &AC,
> +                                                    const FunctionDecl *D) {
> +  QualType ReturnType;
> +  if (const CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(D)) {
> +    ASTContext &CurrContext = AC.getASTContext();
> +    ReturnType = Constructor->getThisType(CurrContext)->getPointeeType();
> +
> +  } else {
> +    ReturnType = cast<FunctionDecl>(D)->getCallResultType();
> +  }
> +
> +  if (D->hasAttr<ReturnTypestateAttr>()) {
> +    const ReturnTypestateAttr *RTSAttr = D->getAttr<ReturnTypestateAttr>();
> +
> +    const CXXRecordDecl *RD = ReturnType->getAsCXXRecordDecl();
> +    if (!RD || !RD->hasAttr<ConsumableAttr>()) {
> +        // FIXME: This should be removed when template instantiation propagates
> +        //        attributes at template specialization definition, not
> +        //        declaration. When it is removed the test needs to be enabled
> +        //        in SemaDeclAttr.cpp.
> +        WarningsHandler.warnReturnTypestateForUnconsumableType(
> +          RTSAttr->getLocation(), ReturnType.getAsString());
> +        ExpectedReturnState = CS_None;
> +
> +    } else {
> +      ExpectedReturnState = mapReturnTypestateAttrState(RTSAttr);
> +    }
> +
> +  } else if (isConsumableType(ReturnType)) {
> +    ExpectedReturnState = mapConsumableAttrState(ReturnType);
> +

Extra newline can be removed

> +  } else {
> +    ExpectedReturnState = CS_None;
> +  }
> +}
> +
>  bool ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock,
>                                    const ConsumedStmtVisitor &Visitor) {
>
> @@ -1051,52 +1117,7 @@
>
>    if (!D) return;
>
> -  // FIXME: This should be removed when template instantiation propagates
> -  //        attributes at template specialization definition, not declaration.
> -  //        When it is removed the test needs to be enabled in SemaDeclAttr.cpp.
> -  QualType ReturnType;
> -  if (const CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(D)) {
> -    ASTContext &CurrContext = AC.getASTContext();
> -    ReturnType = Constructor->getThisType(CurrContext)->getPointeeType();
> -
> -  } else {
> -    ReturnType = D->getCallResultType();
> -  }
> -
> -  // Determine the expected return value.
> -  if (D->hasAttr<ReturnTypestateAttr>()) {
> -
> -    ReturnTypestateAttr *RTSAttr = D->getAttr<ReturnTypestateAttr>();
> -
> -    const CXXRecordDecl *RD = ReturnType->getAsCXXRecordDecl();
> -    if (!RD || !RD->hasAttr<ConsumableAttr>()) {
> -        // FIXME: This branch can be removed with the code above.
> -        WarningsHandler.warnReturnTypestateForUnconsumableType(
> -          RTSAttr->getLocation(), ReturnType.getAsString());
> -        ExpectedReturnState = CS_None;
> -
> -    } else {
> -      switch (RTSAttr->getState()) {
> -      case ReturnTypestateAttr::Unknown:
> -        ExpectedReturnState = CS_Unknown;
> -        break;
> -
> -      case ReturnTypestateAttr::Unconsumed:
> -        ExpectedReturnState = CS_Unconsumed;
> -        break;
> -
> -      case ReturnTypestateAttr::Consumed:
> -        ExpectedReturnState = CS_Consumed;
> -        break;
> -      }
> -    }
> -
> -  } else if (isConsumableType(ReturnType)) {
> -    ExpectedReturnState = CS_Unknown;
> -
> -  } else {
> -    ExpectedReturnState = CS_None;
> -  }
> +  determineExpectedReturnState(AC, D);
>
>    BlockInfo = ConsumedBlockInfo(AC.getCFG());
>
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -968,8 +968,30 @@
>  }
>
>  static void handleConsumableAttr(Sema &S, Decl *D, const AttributeList &Attr) {
> -  if (!checkAttributeNumArgs(S, Attr, 0)) return;
> -
> +  if (!checkAttributeNumArgs(S, Attr, 1)) return;
> +
> +  ConsumableAttr::ConsumedState DefaultState;
> +
> +  if (Attr.isArgIdent(0)) {
> +    StringRef Param = Attr.getArgAsIdent(0)->Ident->getName();
> +
> +    if (Param == "unknown") {
> +      DefaultState = ConsumableAttr::Unknown;
> +    } else if (Param == "consumed") {
> +      DefaultState = ConsumableAttr::Consumed;
> +    } else if (Param == "unconsumed") {
> +      DefaultState = ConsumableAttr::Unconsumed;
> +    } else {
> +      S.Diag(Attr.getLoc(), diag::warn_unknown_consumed_state) << Param;
> +      return;
> +    }

No need for the curly braces on all but the else.

> +
> +  } else {
> +    S.Diag(Attr.getLoc(), diag::err_attribute_argument_type) <<
> +      Attr.getName() << AANT_ArgumentIdentifier;
> +    return;
> +  }
> +
>    if (!isa<CXXRecordDecl>(D)) {
>      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<
>        Attr.getName() << ExpectedClass;
> @@ -977,7 +999,7 @@
>    }
>
>    D->addAttr(::new (S.Context)
> -             ConsumableAttr(Attr.getRange(), S.Context,
> +             ConsumableAttr(Attr.getRange(), S.Context, DefaultState,
>                              Attr.getAttributeSpellingListIndex()));
>  }
>
> Index: test/SemaCXX/warn-consumed-analysis-strict.cpp
> ===================================================================
> --- test/SemaCXX/warn-consumed-analysis-strict.cpp
> +++ test/SemaCXX/warn-consumed-analysis-strict.cpp
> @@ -1,9 +1,9 @@
>  // RUN: %clang_cc1 -fsyntax-only -verify -Wconsumed-strict -std=c++11 %s
>
>  #define CALLABLE_WHEN_UNCONSUMED __attribute__ ((callable_when_unconsumed))
> -#define CONSUMABLE               __attribute__ ((consumable))
> +#define CONSUMABLE(state)        __attribute__ ((consumable(state)))
>  #define CONSUMES                 __attribute__ ((consumes))
> -#define RETURN_TYPESTATE(State)  __attribute__ ((return_typestate(State)))
> +#define RETURN_TYPESTATE(state)  __attribute__ ((return_typestate(state)))
>  #define TESTS_UNCONSUMED         __attribute__ ((tests_unconsumed))
>
>  #define TEST_VAR(Var) Var.isValid()
> @@ -11,15 +11,15 @@
>  typedef decltype(nullptr) nullptr_t;
>
>  template <typename T>
> -class CONSUMABLE ConsumableClass {
> +class CONSUMABLE(unconsumed) ConsumableClass {
>    T var;
>
>    public:
>    ConsumableClass();
>    ConsumableClass(nullptr_t p) RETURN_TYPESTATE(consumed);
> -  ConsumableClass(T val) RETURN_TYPESTATE(unconsumed);
> -  ConsumableClass(ConsumableClass<T> &other) RETURN_TYPESTATE(unconsumed);
> -  ConsumableClass(ConsumableClass<T> &&other) RETURN_TYPESTATE(unconsumed);
> +  ConsumableClass(T val);
> +  ConsumableClass(ConsumableClass<T> &other);
> +  ConsumableClass(ConsumableClass<T> &&other);
>
>    ConsumableClass<T>& operator=(ConsumableClass<T>  &other);
>    ConsumableClass<T>& operator=(ConsumableClass<T> &&other);
> @@ -187,6 +187,10 @@
>    *var;
>  }
>
> +void testFunctionParam0(ConsumableClass<int> &param) {
> +  *param; // expected-warning {{invocation of method 'operator*' on object 'param' while it is in an unknown state}}
> +}
> +
>  void testNoWarnTestFromMacroExpansion() {
>    ConsumableClass<int> var(42);
>
> @@ -195,10 +199,6 @@
>    }
>  }
>
> -void testFunctionParam(ConsumableClass<int> param) {
> -  *param; // expected-warning {{invocation of method 'operator*' on object 'param' while it is in an unknown state}}
> -}
> -
>  void testSimpleForLoop() {
>    ConsumableClass<int> var;
>
> Index: test/SemaCXX/warn-consumed-analysis.cpp
> ===================================================================
> --- test/SemaCXX/warn-consumed-analysis.cpp
> +++ test/SemaCXX/warn-consumed-analysis.cpp
> @@ -1,23 +1,23 @@
>  // RUN: %clang_cc1 -fsyntax-only -verify -Wconsumed -std=c++11 %s
>
>  #define CALLABLE_WHEN_UNCONSUMED __attribute__ ((callable_when_unconsumed))
> -#define CONSUMABLE               __attribute__ ((consumable))
> +#define CONSUMABLE(state)        __attribute__ ((consumable(state)))
>  #define CONSUMES                 __attribute__ ((consumes))
> -#define RETURN_TYPESTATE(State)  __attribute__ ((return_typestate(State)))
> +#define RETURN_TYPESTATE(state)  __attribute__ ((return_typestate(state)))
>  #define TESTS_UNCONSUMED         __attribute__ ((tests_unconsumed))
>
>  typedef decltype(nullptr) nullptr_t;
>
>  template <typename T>
> -class CONSUMABLE ConsumableClass {
> +class CONSUMABLE(unconsumed) ConsumableClass {
>    T var;
>
>    public:
>    ConsumableClass();
>    ConsumableClass(nullptr_t p) RETURN_TYPESTATE(consumed);
>    ConsumableClass(T val) RETURN_TYPESTATE(unconsumed);
> -  ConsumableClass(ConsumableClass<T> &other) RETURN_TYPESTATE(unconsumed);
> -  ConsumableClass(ConsumableClass<T> &&other) RETURN_TYPESTATE(unconsumed);
> +  ConsumableClass(ConsumableClass<T> &other);
> +  ConsumableClass(ConsumableClass<T> &&other);
>
>    ConsumableClass<T>& operator=(ConsumableClass<T>  &other);
>    ConsumableClass<T>& operator=(ConsumableClass<T> &&other);
> @@ -49,7 +49,6 @@
>
>  void baf3(ConsumableClass<int> &&var);
>
> -ConsumableClass<int> returnsUnconsumed() RETURN_TYPESTATE(unconsumed);
>  ConsumableClass<int> returnsUnconsumed() {
>    return ConsumableClass<int>(); // expected-warning {{return value not in expected state; expected 'unconsumed', observed 'consumed'}}
>  }
> @@ -241,7 +240,7 @@
>    if (param.isValid()) {
>      *param;
>    } else {
> -    *param; // expected-warning {{invocation of method 'operator*' on object 'param' while it is in the 'consumed' state}}
> +    *param;
>    }
>
>    param = nullptr;
> Index: test/SemaCXX/warn-consumed-parsing.cpp
> ===================================================================
> --- test/SemaCXX/warn-consumed-parsing.cpp
> +++ test/SemaCXX/warn-consumed-parsing.cpp
> @@ -1,9 +1,9 @@
>  // RUN: %clang_cc1 -fsyntax-only -verify -Wconsumed -std=c++11 %s
>
>  #define CALLABLE_WHEN_UNCONSUMED __attribute__ ((callable_when_unconsumed))
> -#define CONSUMABLE               __attribute__ ((consumable))
> +#define CONSUMABLE(state)        __attribute__ ((consumable(state)))
>  #define CONSUMES                 __attribute__ ((consumes))
> -#define RETURN_TYPESTATE(State)  __attribute__ ((return_typestate(State)))
> +#define RETURN_TYPESTATE(state)  __attribute__ ((return_typestate(state)))
>  #define TESTS_UNCONSUMED         __attribute__ ((tests_unconsumed))
>
>  // FIXME: This warning is not generated if it appears bellow the AttrTester0
> @@ -22,15 +22,15 @@
>  int var0 CONSUMES; // expected-warning {{'consumes' attribute only applies to methods}}
>  int var1 TESTS_UNCONSUMED; // expected-warning {{'tests_unconsumed' attribute only applies to methods}}
>  int var2 CALLABLE_WHEN_UNCONSUMED; // expected-warning {{'callable_when_unconsumed' attribute only applies to methods}}
> -int var3 CONSUMABLE; // expected-warning {{'consumable' attribute only applies to classes}}
> +int var3 CONSUMABLE(consumed); // expected-warning {{'consumable' attribute only applies to classes}}
>  int var4 RETURN_TYPESTATE(consumed); // expected-warning {{'return_typestate' attribute only applies to functions}}
>
>  void function0() CONSUMES; // expected-warning {{'consumes' attribute only applies to methods}}
>  void function1() TESTS_UNCONSUMED; // expected-warning {{'tests_unconsumed' attribute only applies to methods}}
>  void function2() CALLABLE_WHEN_UNCONSUMED; // expected-warning {{'callable_when_unconsumed' attribute only applies to methods}}
> -void function3() CONSUMABLE; // expected-warning {{'consumable' attribute only applies to classes}}
> +void function3() CONSUMABLE(consumed); // expected-warning {{'consumable' attribute only applies to classes}}
>
> -class CONSUMABLE AttrTester1 {
> +class CONSUMABLE(unknown) AttrTester1 {
>    void callableWhenUnconsumed() CALLABLE_WHEN_UNCONSUMED;
>    void consumes()               CONSUMES;
>    bool testsUnconsumed()        TESTS_UNCONSUMED;
> @@ -44,3 +44,5 @@
>    void consumes()               CONSUMES; // expected-warning {{consumed analysis attribute is attached to member of class 'AttrTester2' which isn't marked as consumable}}
>    bool testsUnconsumed()        TESTS_UNCONSUMED; // expected-warning {{consumed analysis attribute is attached to member of class 'AttrTester2' which isn't marked as consumable}}
>  };
> +
> +class CONSUMABLE(42) AttrTester3; // expected-error {{'consumable' attribute requires an identifier}}
>

~Aaron

On Wed, Sep 4, 2013 at 2:11 PM, Delesley Hutchins <delesley at google.com> wrote:
>
>   LGTM, other than the minor nit.
>
>
> ================
> Comment at: lib/Analysis/Consumed.cpp:993
> @@ +992,3 @@
> +  } else {
> +    ReturnType = cast<FunctionDecl>(D)->getCallResultType();
> +  }
> ----------------
> No need for a cast here.
>
>
> http://llvm-reviews.chandlerc.com/D1590



More information about the cfe-commits mailing list