[PATCH] consumable attribute default state

Christian Wailes chriswailes at google.com
Wed Sep 4 13:31:31 PDT 2013


On Wed, Sep 4, 2013 at 12:21 PM, Aaron Ballman <aaron.ballman at gmail.com>wrote:

> 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.
>
>
Because each of the conditionals are multi-line I find the braces and
newlines helpful.  It makes it clearer what lines are part of the
conditional and which are the statements inside the branches.


> > +
> > +  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.
>
>
I feel that if one of the blocks should have braces then they all should.
This makes it consistent, and prevents mistakes if additional code is added
later.  No extra whitespace is added this way, and it just looks weird the
other way.


> > +
> > +  } 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
>

- Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130904/a2e3b10f/attachment.html>


More information about the cfe-commits mailing list