[PATCH] consumable attribute default state
Aaron Ballman
aaron.ballman at gmail.com
Wed Sep 4 13:35:07 PDT 2013
On Wed, Sep 4, 2013 at 4:31 PM, Christian Wailes <chriswailes at google.com> wrote:
>
>
>
> 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.
It's part of the coding style guidelines for patch submission. You
may want to try running clang-format against your code to ensure it
meets all of the style requirements.
>
>>
>> > +
>> > + 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> ¶m) {
>> > + *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
More information about the cfe-commits
mailing list