[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> ¶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
More information about the cfe-commits
mailing list