Thanks for the review, Aaron! Excellent points -- I'll fix those issues in a future patch. FYI, my goal right now is to get all of Chris's stuff working and upstream first; I'll go back and refine the code afterword. You had some good comments earlier regarding use of raw pointers that I want to address too. I appreciate the code review,
-DeLesley On Sat, Oct 12, 2013 at 8:01 AM, Aaron Ballman <[email protected]> wrote: > A couple of points below: > > On Fri, Oct 11, 2013 at 7:03 PM, DeLesley Hutchins <[email protected]> > wrote: >> Author: delesley >> Date: Fri Oct 11 18:03:26 2013 >> New Revision: 192515 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=192515&view=rev >> Log: >> Consumed analysis: replace the consumes attribute with a set_typestate >> attribute. Patch by [email protected]; reviewed and edited by delesley. >> >> Modified: >> cfe/trunk/include/clang/Basic/Attr.td >> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> cfe/trunk/lib/Analysis/Consumed.cpp >> cfe/trunk/lib/Sema/SemaDeclAttr.cpp >> cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp >> cfe/trunk/test/SemaCXX/warn-consumed-parsing.cpp >> >> Modified: cfe/trunk/include/clang/Basic/Attr.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=192515&r1=192514&r2=192515&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/Attr.td (original) >> +++ cfe/trunk/include/clang/Basic/Attr.td Fri Oct 11 18:03:26 2013 >> @@ -967,19 +967,6 @@ def CallableWhen : InheritableAttr { >> ["Unknown", "Consumed", "Unconsumed"]>]; >> } >> >> -def TestsTypestate : InheritableAttr { >> - let Spellings = [GNU<"tests_typestate">]; >> - let Subjects = [CXXMethod]; >> - let Args = [EnumArgument<"TestState", "ConsumedState", >> - ["consumed", "unconsumed"], >> - ["Consumed", "Unconsumed"]>]; >> -} >> - >> -def Consumes : InheritableAttr { >> - let Spellings = [GNU<"consumes">]; >> - let Subjects = [CXXMethod]; >> -} >> - >> def ReturnTypestate : InheritableAttr { >> let Spellings = [GNU<"return_typestate">]; >> let Subjects = [Function]; >> @@ -988,6 +975,22 @@ def ReturnTypestate : InheritableAttr { >> ["Unknown", "Consumed", "Unconsumed"]>]; >> } >> >> +def SetTypestate : InheritableAttr { >> + let Spellings = [GNU<"set_typestate">]; >> + let Subjects = [CXXMethod]; >> + let Args = [EnumArgument<"NewState", "ConsumedState", >> + ["unknown", "consumed", "unconsumed"], >> + ["Unknown", "Consumed", "Unconsumed"]>]; >> +} >> + >> +def TestsTypestate : InheritableAttr { >> + let Spellings = [GNU<"tests_typestate">]; >> + let Subjects = [CXXMethod]; >> + let Args = [EnumArgument<"TestState", "ConsumedState", >> + ["consumed", "unconsumed"], >> + ["Consumed", "Unconsumed"]>]; >> +} >> + >> // Type safety attributes for `void *' pointers and type tags. >> >> def ArgumentWithTypeTag : InheritableAttr { >> >> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=192515&r1=192514&r2=192515&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) >> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Oct 11 18:03:26 >> 2013 >> @@ -2216,8 +2216,8 @@ def warn_attr_on_unconsumable_class : Wa >> def warn_return_typestate_for_unconsumable_type : Warning< >> "return state set for an unconsumable type '%0'">, InGroup<Consumed>, >> DefaultIgnore; >> -def warn_invalid_test_typestate : Warning< >> - "invalid test typestate '%0'">, InGroup<Consumed>, DefaultIgnore; >> +def warn_unknown_consumed_state : Warning< >> + "unknown consumed analysis state '%0'">, InGroup<Consumed>, >> DefaultIgnore; > > This is not needed (more info below). > >> def warn_return_typestate_mismatch : Warning< >> "return value not in expected state; expected '%0', observed '%1'">, >> InGroup<Consumed>, DefaultIgnore; >> >> Modified: cfe/trunk/lib/Analysis/Consumed.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/Consumed.cpp?rev=192515&r1=192514&r2=192515&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Analysis/Consumed.cpp (original) >> +++ cfe/trunk/lib/Analysis/Consumed.cpp Fri Oct 11 18:03:26 2013 >> @@ -157,6 +157,18 @@ static ConsumedState mapConsumableAttrSt >> llvm_unreachable("invalid enum"); >> } >> >> +static ConsumedState mapSetTypestateAttrState(const SetTypestateAttr >> *STAttr) { >> + switch (STAttr->getNewState()) { >> + case SetTypestateAttr::Unknown: >> + return CS_Unknown; >> + case SetTypestateAttr::Unconsumed: >> + return CS_Unconsumed; >> + case SetTypestateAttr::Consumed: >> + return CS_Consumed; >> + } >> + llvm_unreachable("invalid_enum"); >> +} >> + >> static ConsumedState >> mapReturnTypestateAttrState(const ReturnTypestateAttr *RTSAttr) { >> switch (RTSAttr->getState()) { >> @@ -639,8 +651,9 @@ void ConsumedStmtVisitor::VisitCXXMember >> if (isTestingFunction(MethodDecl)) >> PropagationMap.insert(PairType(Call, >> PropagationInfo(PInfo.getVar(), testsFor(MethodDecl)))); >> - else if (MethodDecl->hasAttr<ConsumesAttr>()) >> - StateMap->setState(PInfo.getVar(), consumed::CS_Consumed); >> + else if (MethodDecl->hasAttr<SetTypestateAttr>()) >> + StateMap->setState(PInfo.getVar(), >> + >> mapSetTypestateAttrState(MethodDecl->getAttr<SetTypestateAttr>())); >> } >> } >> } >> @@ -728,8 +741,9 @@ void ConsumedStmtVisitor::VisitCXXOperat >> if (isTestingFunction(FunDecl)) >> PropagationMap.insert(PairType(Call, >> PropagationInfo(PInfo.getVar(), testsFor(FunDecl)))); >> - else if (FunDecl->hasAttr<ConsumesAttr>()) >> - StateMap->setState(PInfo.getVar(), consumed::CS_Consumed); >> + else if (FunDecl->hasAttr<SetTypestateAttr>()) >> + StateMap->setState(PInfo.getVar(), >> + mapSetTypestateAttrState(FunDecl->getAttr<SetTypestateAttr>())); >> } >> } >> } >> >> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=192515&r1=192514&r2=192515&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Oct 11 18:03:26 2013 >> @@ -1026,20 +1026,6 @@ static bool checkForConsumableClass(Sema >> return true; >> } >> >> -static void handleConsumesAttr(Sema &S, Decl *D, const AttributeList &Attr) >> { >> - if (!isa<CXXMethodDecl>(D)) { >> - S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) << >> - Attr.getName() << ExpectedMethod; >> - return; >> - } >> - >> - if (!checkForConsumableClass(S, cast<CXXMethodDecl>(D), Attr)) >> - return; >> - >> - D->addAttr(::new (S.Context) >> - ConsumesAttr(Attr.getRange(), S.Context, >> - Attr.getAttributeSpellingListIndex())); >> -} >> >> static void handleCallableWhenAttr(Sema &S, Decl *D, >> const AttributeList &Attr) { >> @@ -1080,44 +1066,6 @@ static void handleCallableWhenAttr(Sema >> } >> >> >> -static void handleTestsTypestateAttr(Sema &S, Decl *D, >> - const AttributeList &Attr) { >> - if (!checkAttributeNumArgs(S, Attr, 1)) return; >> - >> - if (!isa<CXXMethodDecl>(D)) { >> - S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) << >> - Attr.getName() << ExpectedMethod; >> - return; >> - } >> - >> - if (!checkForConsumableClass(S, cast<CXXMethodDecl>(D), Attr)) >> - return; >> - >> - TestsTypestateAttr::ConsumedState TestState; >> - >> - if (Attr.isArgIdent(0)) { >> - StringRef Param = Attr.getArgAsIdent(0)->Ident->getName(); >> - >> - if (Param == "consumed") { >> - TestState = TestsTypestateAttr::Consumed; >> - } else if (Param == "unconsumed") { >> - TestState = TestsTypestateAttr::Unconsumed; >> - } else { >> - S.Diag(Attr.getLoc(), diag::warn_invalid_test_typestate) << Param; >> - return; >> - } >> - >> - } else { >> - S.Diag(Attr.getLoc(), diag::err_attribute_argument_type) << >> - Attr.getName() << AANT_ArgumentIdentifier; >> - return; >> - } >> - >> - D->addAttr(::new (S.Context) >> - TestsTypestateAttr(Attr.getRange(), S.Context, TestState, >> - Attr.getAttributeSpellingListIndex())); >> -} >> - >> static void handleReturnTypestateAttr(Sema &S, Decl *D, >> const AttributeList &Attr) { >> ReturnTypestateAttr::ConsumedState ReturnState; >> @@ -1168,6 +1116,84 @@ static void handleReturnTypestateAttr(Se >> Attr.getAttributeSpellingListIndex())); >> } >> >> + >> +static void handleSetTypestateAttr(Sema &S, Decl *D, const AttributeList >> &Attr) { >> + if (!checkAttributeNumArgs(S, Attr, 1)) return; >> + >> + if (!isa<CXXMethodDecl>(D)) { >> + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) << >> + Attr.getName() << ExpectedMethod; >> + return; >> + } >> + >> + if (!checkForConsumableClass(S, cast<CXXMethodDecl>(D), Attr)) >> + return; >> + >> + SetTypestateAttr::ConsumedState NewState; >> + >> + if (Attr.isArgIdent(0)) { >> + StringRef Param = Attr.getArgAsIdent(0)->Ident->getName(); >> + >> + if (Param == "unknown") { >> + NewState = SetTypestateAttr::Unknown; >> + } else if (Param == "consumed") { >> + NewState = SetTypestateAttr::Consumed; >> + } else if (Param == "unconsumed") { >> + NewState = SetTypestateAttr::Unconsumed; >> + } else { >> + S.Diag(Attr.getLoc(), diag::warn_unknown_consumed_state) << Param; >> + return; >> + } >> + >> + } else { >> + S.Diag(Attr.getLoc(), diag::err_attribute_argument_type) << >> + Attr.getName() << AANT_ArgumentIdentifier; >> + return; >> + } > > This whole block (from isArgIdent down) can be replaced with > ConvertStrToTypeSetState (or something akin to that) -- it's td > generated code for converting enumerations to their proper value, > including many diagnostics. As an example, here's visibility: > > VisibilityAttr::VisibilityType type; > if (!VisibilityAttr::ConvertStrToVisibilityType(TypeStr, type)) { > S.Diag(LiteralLoc, diag::warn_attribute_type_not_supported) > << Attr.getName() << TypeStr; > return; > } > > You should be using the warn_attribute_type_not_support diagnostic for > compatibility. > >> + >> + D->addAttr(::new (S.Context) >> + SetTypestateAttr(Attr.getRange(), S.Context, NewState, >> + Attr.getAttributeSpellingListIndex())); >> +} >> + >> +static void handleTestsTypestateAttr(Sema &S, Decl *D, >> + const AttributeList &Attr) { >> + if (!checkAttributeNumArgs(S, Attr, 1)) return; >> + >> + if (!isa<CXXMethodDecl>(D)) { >> + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) << >> + Attr.getName() << ExpectedMethod; >> + return; >> + } >> + >> + if (!checkForConsumableClass(S, cast<CXXMethodDecl>(D), Attr)) >> + return; >> + >> + TestsTypestateAttr::ConsumedState TestState; >> + >> + if (Attr.isArgIdent(0)) { >> + StringRef Param = Attr.getArgAsIdent(0)->Ident->getName(); >> + >> + if (Param == "consumed") { >> + TestState = TestsTypestateAttr::Consumed; >> + } else if (Param == "unconsumed") { >> + TestState = TestsTypestateAttr::Unconsumed; >> + } else { >> + S.Diag(Attr.getLoc(), diag::warn_unknown_consumed_state) << Param; >> + return; >> + } >> + >> + } else { >> + S.Diag(Attr.getLoc(), diag::err_attribute_argument_type) << >> + Attr.getName() << AANT_ArgumentIdentifier; >> + return; >> + } > > Same comments apply here as above. > >> + >> + D->addAttr(::new (S.Context) >> + TestsTypestateAttr(Attr.getRange(), S.Context, TestState, >> + Attr.getAttributeSpellingListIndex())); >> +} >> + >> static void handleExtVectorTypeAttr(Sema &S, Scope *scope, Decl *D, >> const AttributeList &Attr) { >> TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D); >> @@ -4793,18 +4819,18 @@ static void ProcessDeclAttribute(Sema &S >> case AttributeList::AT_Consumable: >> handleConsumableAttr(S, D, Attr); >> break; >> - case AttributeList::AT_Consumes: >> - handleConsumesAttr(S, D, Attr); >> - break; >> case AttributeList::AT_CallableWhen: >> handleCallableWhenAttr(S, D, Attr); >> break; >> - case AttributeList::AT_TestsTypestate: >> - handleTestsTypestateAttr(S, D, Attr); >> - break; >> case AttributeList::AT_ReturnTypestate: >> handleReturnTypestateAttr(S, D, Attr); >> break; >> + case AttributeList::AT_SetTypestate: >> + handleSetTypestateAttr(S, D, Attr); >> + break; >> + case AttributeList::AT_TestsTypestate: >> + handleTestsTypestateAttr(S, D, Attr); >> + break; >> >> // Type safety attributes. >> case AttributeList::AT_ArgumentWithTypeTag: >> >> Modified: cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp?rev=192515&r1=192514&r2=192515&view=diff >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp (original) >> +++ cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp Fri Oct 11 18:03:26 >> 2013 >> @@ -4,7 +4,7 @@ >> >> #define CALLABLE_WHEN(...) __attribute__ ((callable_when(__VA_ARGS__))) >> #define CONSUMABLE(state) __attribute__ ((consumable(state))) >> -#define CONSUMES __attribute__ ((consumes)) >> +#define SET_TYPESTATE(state) __attribute__ ((set_typestate(state))) >> #define RETURN_TYPESTATE(state) __attribute__ ((return_typestate(state))) >> #define TESTS_TYPESTATE(state) __attribute__ ((tests_typestate(state))) >> >> @@ -23,7 +23,7 @@ public: >> >> ConsumableClass<T>& operator=(ConsumableClass<T> &other); >> ConsumableClass<T>& operator=(ConsumableClass<T> &&other); >> - ConsumableClass<T>& operator=(nullptr_t) CONSUMES; >> + ConsumableClass<T>& operator=(nullptr_t) SET_TYPESTATE(consumed); >> >> template <typename U> >> ConsumableClass<T>& operator=(ConsumableClass<U> &other); >> @@ -31,7 +31,7 @@ public: >> template <typename U> >> ConsumableClass<T>& operator=(ConsumableClass<U> &&other); >> >> - void operator()(int a) CONSUMES; >> + void operator()(int a) SET_TYPESTATE(consumed); >> void operator*() const CALLABLE_WHEN("unconsumed"); >> void unconsumedCall() const CALLABLE_WHEN("unconsumed"); >> void callableWhenUnknown() const CALLABLE_WHEN("unconsumed", "unknown"); >> @@ -44,7 +44,8 @@ public: >> void constCall() const; >> void nonconstCall(); >> >> - void consume() CONSUMES; >> + void consume() SET_TYPESTATE(consumed); >> + void unconsume() SET_TYPESTATE(unconsumed); >> }; >> >> class CONSUMABLE(unconsumed) DestructorTester { >> @@ -484,7 +485,7 @@ void testConditionalMerge() { >> *var; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var' while it is in the 'consumed' state}} >> } >> >> -void testConsumes0() { >> +void testSetTypestate() { >> ConsumableClass<int> var(42); >> >> *var; >> @@ -492,15 +493,19 @@ void testConsumes0() { >> var.consume(); >> >> *var; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var' while it is in the 'consumed' state}} >> + >> + var.unconsume(); >> + >> + *var; >> } >> >> -void testConsumes1() { >> +void testConsumes0() { >> ConsumableClass<int> var(nullptr); >> >> *var; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var' while it is in the 'consumed' state}} >> } >> >> -void testConsumes2() { >> +void testConsumes1() { >> ConsumableClass<int> var(42); >> >> var.unconsumedCall(); >> >> Modified: cfe/trunk/test/SemaCXX/warn-consumed-parsing.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-consumed-parsing.cpp?rev=192515&r1=192514&r2=192515&view=diff >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/warn-consumed-parsing.cpp (original) >> +++ cfe/trunk/test/SemaCXX/warn-consumed-parsing.cpp Fri Oct 11 18:03:26 2013 >> @@ -1,10 +1,10 @@ >> // RUN: %clang_cc1 -fsyntax-only -verify -Wconsumed -std=c++11 %s >> >> #define CALLABLE_WHEN(...) __attribute__ ((callable_when(__VA_ARGS__))) >> -#define CONSUMABLE(state) __attribute__ ((consumable(state))) >> -#define CONSUMES __attribute__ ((consumes)) >> -#define RETURN_TYPESTATE(state) __attribute__ ((return_typestate(state))) >> -#define TESTS_TYPESTATE(state) __attribute__ ((tests_typestate(state))) >> +#define CONSUMABLE(state) __attribute__ ((consumable(state))) >> +#define SET_TYPESTATE(state) __attribute__ ((set_typestate(state))) >> +#define RETURN_TYPESTATE(state) __attribute__ ((return_typestate(state))) >> +#define TESTS_TYPESTATE(state) __attribute__ ((tests_typestate(state))) >> >> // FIXME: This test is here because the warning is issued by the Consumed >> // analysis, not SemaDeclAttr. The analysis won't run after an error >> @@ -17,27 +17,27 @@ int returnTypestateForUnconsumable() { >> } >> >> class AttrTester0 { >> - void consumes() __attribute__ ((consumes(42))); // expected-error >> {{attribute takes no arguments}} >> + void consumes() __attribute__ ((set_typestate())); // >> expected-error {{attribute takes one argument}} >> bool testsUnconsumed() __attribute__ ((tests_typestate())); // >> expected-error {{attribute takes one argument}} >> void callableWhen() __attribute__ ((callable_when())); // >> expected-error {{attribute takes at least 1 argument}} >> }; >> >> -int var0 CONSUMES; // expected-warning {{'consumes' attribute only applies >> to methods}} >> +int var0 SET_TYPESTATE(consumed); // expected-warning {{'set_typestate' >> attribute only applies to methods}} >> int var1 TESTS_TYPESTATE(consumed); // expected-warning {{'tests_typestate' >> attribute only applies to methods}} >> -int var2 CALLABLE_WHEN(42); // expected-warning {{'callable_when' attribute >> only applies to methods}} >> +int var2 CALLABLE_WHEN("consumed"); // expected-warning {{'callable_when' >> attribute only applies to methods}} >> 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 function0() SET_TYPESTATE(consumed); // expected-warning >> {{'set_typestate' attribute only applies to methods}} >> void function1() TESTS_TYPESTATE(consumed); // expected-warning >> {{'tests_typestate' attribute only applies to methods}} >> -void function2() CALLABLE_WHEN(42); // expected-warning {{'callable_when' >> attribute only applies to methods}} >> +void function2() CALLABLE_WHEN("consumed"); // expected-warning >> {{'callable_when' attribute only applies to methods}} >> void function3() CONSUMABLE(consumed); // expected-warning {{'consumable' >> attribute only applies to classes}} >> >> class CONSUMABLE(unknown) AttrTester1 { >> void callableWhen0() CALLABLE_WHEN("unconsumed"); >> void callableWhen1() CALLABLE_WHEN(42); // expected-error >> {{'callable_when' attribute requires a string}} >> void callableWhen2() CALLABLE_WHEN("foo"); // expected-warning >> {{'callable_when' attribute argument not supported: foo}} >> - void consumes() CONSUMES; >> + void consumes() SET_TYPESTATE(consumed); >> bool testsUnconsumed() TESTS_TYPESTATE(consumed); >> }; >> >> @@ -46,7 +46,7 @@ AttrTester1 returnTypestateTester1() RET >> >> class AttrTester2 { >> void callableWhen() CALLABLE_WHEN("unconsumed"); // expected-warning >> {{consumed analysis attribute is attached to member of class 'AttrTester2' >> which isn't marked as consumable}} >> - void consumes() CONSUMES; // expected-warning {{consumed analysis >> attribute is attached to member of class 'AttrTester2' which isn't marked as >> consumable}} >> + void consumes() SET_TYPESTATE(consumed); // expected-warning >> {{consumed analysis attribute is attached to member of class 'AttrTester2' >> which isn't marked as consumable}} >> bool testsUnconsumed() TESTS_TYPESTATE(consumed); // expected-warning >> {{consumed analysis attribute is attached to member of class 'AttrTester2' >> which isn't marked as consumable}} >> }; >> >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > ~Aaron -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
