<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 2, 2013 at 3:54 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Sep 2, 2013 at 6:45 PM, Christian Wailes <<a href="mailto:chriswailes@google.com">chriswailes@google.com</a>> wrote:<br>
> Notes below. Updated patch for your refactoring of AttributeList coming<br>
> soon.<br>
><br>
> - Chris<br>
><br>
><br>
> On Sun, Sep 1, 2013 at 7:50 AM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>><br>
> wrote:<br>
>><br>
>> No major complaints, but some comments are interspersed below.<br>
>><br>
>> > Index: include/clang/Analysis/Analyses/Consumed.h<br>
>> > ===================================================================<br>
>> > --- include/clang/Analysis/Analyses/Consumed.h<br>
>> > +++ include/clang/Analysis/Analyses/Consumed.h<br>
>> > @@ -39,11 +39,27 @@<br>
>> ><br>
>> > /// \brief Emit the warnings and notes left by the analysis.<br>
>> > virtual void emitDiagnostics() {}<br>
>> > -<br>
>> > +<br>
>> > + // FIXME: This can be removed when the attr propagation fix for<br>
>> > templated<br>
>> > + // classes lands.<br>
>> > + /// \brief Warn about return typestates set for unconsumable types.<br>
>> > + ///<br>
>> > + /// \param Loc -- The location of the attributes.<br>
>> > + ///<br>
>> > + /// \param TypeName -- The name of the unconsumable type.<br>
>> > + virtual void warnReturnTypestateForUnconsumableType(SourceLocation<br>
>> > Loc,<br>
>> > + StringRef<br>
>> > TypeName) {}<br>
>> > +<br>
>> > + /// \brief Warn about return typestate mismatches.<br>
>> > + /// \param Loc -- The SourceLocation of the return statement.<br>
>> > + virtual void warnReturnTypestateMismatch(SourceLocation Loc) {}<br>
>> > +<br>
>> > /// \brief Warn about unnecessary-test errors.<br>
>> > /// \param VariableName -- The name of the variable that holds the<br>
>> > unique<br>
>> > /// value.<br>
>> > ///<br>
>> > + /// \param VariableState -- The known state of the value.<br>
>> > + ///<br>
>> > /// \param Loc -- The SourceLocation of the unnecessary test.<br>
>> > virtual void warnUnnecessaryTest(StringRef VariableName,<br>
>> > StringRef VariableState,<br>
>> > @@ -170,6 +186,8 @@<br>
>> > ConsumedBlockInfo BlockInfo;<br>
>> > ConsumedStateMap *CurrStates;<br>
>> ><br>
>> > + ConsumedState ExpectedReturnState;<br>
>> > +<br>
>> > bool hasConsumableAttributes(const CXXRecordDecl *RD);<br>
>> > bool splitState(const CFGBlock *CurrBlock,<br>
>> > const ConsumedStmtVisitor &Visitor);<br>
>> > @@ -181,6 +199,8 @@<br>
>> > ConsumedAnalyzer(ConsumedWarningsHandlerBase &WarningsHandler)<br>
>> > : WarningsHandler(WarningsHandler) {}<br>
>> ><br>
>> > + ConsumedState getExpectedReturnState() { return<br>
>> > ExpectedReturnState; }<br>
>><br>
>> Method can be const<br>
>><br>
>> > +<br>
>> > /// \brief Check a function's CFG for consumed violations.<br>
>> > ///<br>
>> > /// We traverse the blocks in the CFG, keeping track of the state<br>
>> > of each<br>
>> > Index: include/clang/Basic/Attr.td<br>
>> > ===================================================================<br>
>> > --- include/clang/Basic/Attr.td<br>
>> > +++ include/clang/Basic/Attr.td<br>
>> > @@ -953,6 +953,14 @@<br>
>> > let Subjects = [CXXMethod];<br>
>> > }<br>
>> ><br>
>> > +def ReturnTypestate : InheritableAttr {<br>
>> > + let Spellings = [GNU<"return_typestate">];<br>
>> > + let Subjects = [Function];<br>
>> > + let Args = [EnumArgument<"State", "ConsumedState",<br>
>> > + ["unknown", "consumed", "unconsumed"],<br>
>> > + ["Unknown", "Consumed", "Unconsumed"]>];<br>
>> > +}<br>
>> > +<br>
>> > // Type safety attributes for `void *' pointers and type tags.<br>
>> ><br>
>> > def ArgumentWithTypeTag : InheritableAttr {<br>
>> > Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > ===================================================================<br>
>> > --- include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > +++ include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > @@ -2190,8 +2190,16 @@<br>
>> > "invocation of method '%0' on a temporary object while it is in the "<br>
>> > "'consumed' state">, InGroup<Consumed>, DefaultIgnore;<br>
>> > def warn_attr_on_unconsumable_class : Warning<<br>
>> > - "consumed analysis attribute is attached to class '%0' which isn't<br>
>> > marked "<br>
>> > - "as consumable">, InGroup<Consumed>, DefaultIgnore;<br>
>> > + "consumed analysis attribute is attached to member of class '%0'<br>
>> > which isn't "<br>
>> > + "marked as consumable">, InGroup<Consumed>, DefaultIgnore;<br>
>> > +def warn_return_typestate_for_unconsumable_type : Warning<<br>
>> > + "return state set for an unconsumable type '%0'">, InGroup<Consumed>,<br>
>> > + DefaultIgnore;<br>
>> > +def warn_unknown_consumed_state : Warning<<br>
>> > + "unknown consumed analysis state">, InGroup<Consumed>, DefaultIgnore;<br>
>><br>
>> It would be good to list the state.<br>
>><br>
>> > +def warn_return_typestate_mismatch : Warning<<br>
>> > + "return value not in expected state">, InGroup<Consumed>,<br>
>> > + DefaultIgnore;<br>
>><br>
>> It might make for a better diagnostics to tell the user what state the<br>
>> return value is in, and what the expected state was.<br>
>><br>
><br>
> I was planning on adding that information in a note as I couldn't think of a<br>
> nice, concise way of expressing it in the warning. This will also allow me<br>
> to point to where the return typestate is declared.<br>
<br>
</div></div>It could even be as simple as: inconsistent return value state;<br>
expected %0, got %1<br>
<br>
Though I'm sure someone better with words can improve upon that. ;-)<br></blockquote><div><br></div><div>Done.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
>><br>
>> ><br>
>> > // ConsumedStrict warnings<br>
>> > def warn_use_in_unknown_state : Warning<<br>
>> > Index: lib/Analysis/Consumed.cpp<br>
>> > ===================================================================<br>
>> > --- lib/Analysis/Consumed.cpp<br>
>> > +++ lib/Analysis/Consumed.cpp<br>
>> > @@ -31,6 +31,7 @@<br>
>> > #include "llvm/Support/Compiler.h"<br>
>> > #include "llvm/Support/raw_ostream.h"<br>
>> ><br>
>> > +// TODO: Add notes about the actual and expected state for<br>
>> > // TODO: Correctly identify unreachable blocks when chaining boolean<br>
>> > operators.<br>
>> > // TODO: Warn about unreachable code.<br>
>> > // TODO: Switch to using a bitmap to track unreachable blocks.<br>
>> > @@ -256,6 +257,8 @@<br>
>> > void forwardInfo(const Stmt *From, const Stmt *To);<br>
>> > void handleTestingFunctionCall(const CallExpr *Call, const VarDecl<br>
>> > *Var);<br>
>> > bool isLikeMoveAssignment(const CXXMethodDecl *MethodDecl);<br>
>> > + void propagateReturnType(const Stmt *Call, const FunctionDecl *Fun,<br>
>> > + QualType ReturnType);<br>
>> ><br>
>> > public:<br>
>> ><br>
>> > @@ -272,6 +275,7 @@<br>
>> > void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr<br>
>> > *Temp);<br>
>> > void VisitMemberExpr(const MemberExpr *MExpr);<br>
>> > void VisitParmVarDecl(const ParmVarDecl *Param);<br>
>> > + void VisitReturnStmt(const ReturnStmt *Ret);<br>
>> > void VisitUnaryOperator(const UnaryOperator *UOp);<br>
>> > void VisitVarDecl(const VarDecl *Var);<br>
>> ><br>
>> > @@ -373,6 +377,34 @@<br>
>> ><br>
>> > MethodDecl->getParamDecl(0)->getType()->isRValueReferenceType());<br>
>> > }<br>
>> ><br>
>> > +void ConsumedStmtVisitor::propagateReturnType(const Stmt *Call,<br>
>> > + const FunctionDecl *Fun,<br>
>> > + QualType ReturnType) {<br>
>> > + if (isConsumableType(ReturnType)) {<br>
>> > + if (Fun->hasAttr<ReturnTypestateAttr>()) {<br>
>> > + switch (Fun->getAttr<ReturnTypestateAttr>()->getState()) {<br>
>> > + case ReturnTypestateAttr::Unconsumed:<br>
>> > + PropagationMap.insert(PairType(Call,<br>
>> > +<br>
>> > PropagationInfo(consumed::CS_Unconsumed)));<br>
>> > + break;<br>
>> > +<br>
>> > + case ReturnTypestateAttr::Consumed:<br>
>> > + PropagationMap.insert(PairType(Call,<br>
>> > + PropagationInfo(consumed::CS_Consumed)));<br>
>> > + break;<br>
>> > +<br>
>> > + case ReturnTypestateAttr::Unknown:<br>
>> > + PropagationMap.insert(PairType(Call,<br>
>> > + PropagationInfo(consumed::CS_Unknown)));<br>
>> > + break;<br>
>> > + }<br>
>> > + } else {<br>
>> > + PropagationMap.insert(PairType(Call,<br>
>> > + PropagationInfo(consumed::CS_Unknown)));<br>
>> > + }<br>
>> > + }<br>
>> > +}<br>
>><br>
>> There's a fair amount of duplication there. The only variability is<br>
>> what the PropagationInfo enumerant is, so I think it'd be more clear<br>
>> to have a single call to insert instead of multiple.<br>
>><br>
><br>
> Already done in a future patch. Backported into the updated patch.<br>
><br>
>><br>
>> > +<br>
>> > void ConsumedStmtVisitor::Visit(const Stmt *StmtNode) {<br>
>> ><br>
>> > ConstStmtVisitor<ConsumedStmtVisitor>::Visit(StmtNode);<br>
>> > @@ -469,6 +501,8 @@<br>
>> > StateMap->setState(PInfo.getVar(), consumed::CS_Unknown);<br>
>> > }<br>
>> > }<br>
>> > +<br>
>> > + propagateReturnType(Call, FunDecl, FunDecl->getCallResultType());<br>
>> > }<br>
>> > }<br>
>> ><br>
>> > @@ -483,8 +517,7 @@<br>
>> > QualType ThisType =<br>
>> > Constructor->getThisType(CurrContext)->getPointeeType();<br>
>> ><br>
>> > if (isConsumableType(ThisType)) {<br>
>> > - if (Constructor->hasAttr<ConsumesAttr>() ||<br>
>> > - Constructor->isDefaultConstructor()) {<br>
>> > + if (Constructor->isDefaultConstructor()) {<br>
>> ><br>
>> > PropagationMap.insert(PairType(Call,<br>
>> > PropagationInfo(consumed::CS_Consumed)));<br>
>> > @@ -513,8 +546,7 @@<br>
>> > PropagationMap.insert(PairType(Call, Entry->second));<br>
>> ><br>
>> > } else {<br>
>> > - PropagationMap.insert(PairType(Call,<br>
>> > - PropagationInfo(consumed::CS_Unconsumed)));<br>
>> > + propagateReturnType(Call, Constructor, ThisType);<br>
>> > }<br>
>> > }<br>
>> > }<br>
>> > @@ -677,6 +709,23 @@<br>
>> > StateMap->setState(Param, consumed::CS_Unknown);<br>
>> > }<br>
>> ><br>
>> > +void ConsumedStmtVisitor::VisitReturnStmt(const ReturnStmt *Ret) {<br>
>> > + if (Analyzer.getExpectedReturnState()) {<br>
>> > + InfoEntry Entry = PropagationMap.find(Ret->getRetValue());<br>
>> > +<br>
>> > + if (Entry != PropagationMap.end()) {<br>
>> > + assert(Entry->second.isState() || Entry->second.isVar());<br>
>> > +<br>
>> > + ConsumedState RetState = Entry->second.isState() ?<br>
>> > + Entry->second.getState() :<br>
>> > StateMap->getState(Entry->second.getVar());<br>
>> > +<br>
>> > + if (RetState != Analyzer.getExpectedReturnState())<br>
>> > + Analyzer.WarningsHandler.warnReturnTypestateMismatch(<br>
>> > + Ret->getReturnLoc());<br>
>> > + }<br>
>> > + }<br>
>> > +}<br>
>> > +<br>
>> > void ConsumedStmtVisitor::VisitUnaryOperator(const UnaryOperator *UOp)<br>
>> > {<br>
>> > InfoEntry Entry =<br>
>> > PropagationMap.find(UOp->getSubExpr()->IgnoreParens());<br>
>> > if (Entry == PropagationMap.end()) return;<br>
>> > @@ -997,6 +1046,53 @@<br>
>> ><br>
>> > if (!D) return;<br>
>> ><br>
>> > + // FIXME: This should be removed when template instantiation<br>
>> > propagates<br>
>> > + // attributes at template specialization definition, not<br>
>> > declaration.<br>
>> > + // When it is removed the test needs to be enabled in<br>
>> > SemaDeclAttr.cpp.<br>
>> > + QualType ReturnType;<br>
>> > + if (const CXXConstructorDecl *Constructor =<br>
>> > dyn_cast<CXXConstructorDecl>(D)) {<br>
>> > + ASTContext &CurrContext = AC.getASTContext();<br>
>> > + ReturnType =<br>
>> > Constructor->getThisType(CurrContext)->getPointeeType();<br>
>> > +<br>
>> > + } else {<br>
>> > + ReturnType = cast<FunctionDecl>(D)->getCallResultType();<br>
>> > + }<br>
>> > +<br>
>> > + // Determine the expected return value.<br>
>> > + if (D->hasAttr<ReturnTypestateAttr>()) {<br>
>> > +<br>
>> > + ReturnTypestateAttr *RTSAttr = D->getAttr<ReturnTypestateAttr>();<br>
>> > +<br>
>> > + const CXXRecordDecl *RD = ReturnType->getAsCXXRecordDecl();<br>
>> > + if (!RD || !RD->hasAttr<ConsumableAttr>()) {<br>
>> > + // FIXME: This branch can be removed with the code above.<br>
>> > + WarningsHandler.warnReturnTypestateForUnconsumableType(<br>
>> > + RTSAttr->getLocation(), ReturnType.getAsString());<br>
>> > + ExpectedReturnState = CS_None;<br>
>> > +<br>
>> > + } else {<br>
>> > + switch (RTSAttr->getState()) {<br>
>> > + case ReturnTypestateAttr::Unknown:<br>
>> > + ExpectedReturnState = CS_Unknown;<br>
>> > + break;<br>
>> > +<br>
>> > + case ReturnTypestateAttr::Unconsumed:<br>
>> > + ExpectedReturnState = CS_Unconsumed;<br>
>> > + break;<br>
>> > +<br>
>> > + case ReturnTypestateAttr::Consumed:<br>
>> > + ExpectedReturnState = CS_Consumed;<br>
>> > + break;<br>
>> > + }<br>
>> > + }<br>
>> > +<br>
>> > + } else if (isConsumableType(ReturnType)) {<br>
>> > + ExpectedReturnState = CS_Unknown;<br>
>> > +<br>
>> > + } else {<br>
>> > + ExpectedReturnState = CS_None;<br>
>> > + }<br>
>> > +<br>
>> > BlockInfo = ConsumedBlockInfo(AC.getCFG());<br>
>> ><br>
>> > PostOrderCFGView *SortedGraph = AC.getAnalysis<PostOrderCFGView>();<br>
>> > Index: lib/Sema/AnalysisBasedWarnings.cpp<br>
>> > ===================================================================<br>
>> > --- lib/Sema/AnalysisBasedWarnings.cpp<br>
>> > +++ lib/Sema/AnalysisBasedWarnings.cpp<br>
>> > @@ -1445,11 +1445,21 @@<br>
>> > }<br>
>> > }<br>
>> ><br>
>> > - /// Warn about unnecessary-test errors.<br>
>> > - /// \param VariableName -- The name of the variable that holds the<br>
>> > unique<br>
>> > - /// value.<br>
>> > - ///<br>
>> > - /// \param Loc -- The SourceLocation of the unnecessary test.<br>
>> > + void warnReturnTypestateForUnconsumableType(SourceLocation Loc,<br>
>> > + StringRef TypeName) {<br>
>> > + PartialDiagnosticAt Warning(Loc, S.PDiag(<br>
>> > + diag::warn_return_typestate_for_unconsumable_type) << TypeName);<br>
>><br>
>> You are missing an enabled test case for this warning.<br>
>><br>
><br>
> The test for this is on line 42 of warn-consumed-parsing.cpp. It is<br>
> currently commented out because an issue with attributes being incorrectly<br>
> propagated for template specializations is preventing us from checking for<br>
> this problem when parsing the attributes.<br>
<br>
</div></div>Yes, that's why I mentioned an enabled test. But it seems like you<br>
could write a test that doesn't use template specialization?<br>
<div><div class="h5"><br></div></div></blockquote><div><br></div><div>Can you elaborate on what you mean by an 'enabled test'? And while we could test for non-template specialized cases in SemaDeclAttr.cpp now, I was told to comment out that code and check for the warning in the ConsumedAnalyzer by DeLesely.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> There are some FIXMEs running<br>
> around telling someone to un-comment the test and move some code around once<br>
> the issue is resolved. Specifically, this warning function will be removed<br>
> and the warning will be issued from SemaDeclAttr directly.<br>
><br>
>><br>
>> > +<br>
>> > + Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));<br>
>> > + }<br>
>> > +<br>
>> > + void warnReturnTypestateMismatch(SourceLocation Loc) {<br>
>> > + PartialDiagnosticAt Warning(Loc, S.PDiag(<br>
>> > + diag::warn_return_typestate_mismatch));<br>
>> > +<br>
>> > + Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));<br>
>> > + }<br>
>> > +<br>
>> > void warnUnnecessaryTest(StringRef VariableName, StringRef<br>
>> > VariableState,<br>
>> > SourceLocation Loc) {<br>
>> ><br>
>> > @@ -1459,11 +1469,6 @@<br>
>> > Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));<br>
>> > }<br>
>> ><br>
>> > - /// Warn about use-while-consumed errors.<br>
>> > - /// \param MethodName -- The name of the method that was incorrectly<br>
>> > - /// invoked.<br>
>> > - ///<br>
>> > - /// \param Loc -- The SourceLocation of the method invocation.<br>
>> > void warnUseOfTempWhileConsumed(StringRef MethodName, SourceLocation<br>
>> > Loc) {<br>
>> ><br>
>> > PartialDiagnosticAt Warning(Loc, S.PDiag(<br>
>> > @@ -1472,11 +1477,6 @@<br>
>> > Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));<br>
>> > }<br>
>> ><br>
>> > - /// Warn about use-in-unknown-state errors.<br>
>> > - /// \param MethodName -- The name of the method that was incorrectly<br>
>> > - /// invoked.<br>
>> > - ///<br>
>> > - /// \param Loc -- The SourceLocation of the method invocation.<br>
>> > void warnUseOfTempInUnknownState(StringRef MethodName, SourceLocation<br>
>> > Loc) {<br>
>> ><br>
>> > PartialDiagnosticAt Warning(Loc, S.PDiag(<br>
>> > @@ -1485,14 +1485,6 @@<br>
>> > Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));<br>
>> > }<br>
>> ><br>
>> > - /// Warn about use-while-consumed errors.<br>
>> > - /// \param MethodName -- The name of the method that was incorrectly<br>
>> > - /// invoked.<br>
>> > - ///<br>
>> > - /// \param VariableName -- The name of the variable that holds the<br>
>> > unique<br>
>> > - /// value.<br>
>> > - ///<br>
>> > - /// \param Loc -- The SourceLocation of the method invocation.<br>
>> > void warnUseWhileConsumed(StringRef MethodName, StringRef<br>
>> > VariableName,<br>
>> > SourceLocation Loc) {<br>
>> ><br>
>> > @@ -1502,14 +1494,6 @@<br>
>> > Warnings.push_back(DelayedDiag(Warning, OptionalNotes()));<br>
>> > }<br>
>> ><br>
>> > - /// Warn about use-in-unknown-state errors.<br>
>> > - /// \param MethodName -- The name of the method that was incorrectly<br>
>> > - /// invoked.<br>
>> > - ///<br>
>> > - /// \param VariableName -- The name of the variable that holds the<br>
>> > unique<br>
>> > - /// value.<br>
>> > - ///<br>
>> > - /// \param Loc -- The SourceLocation of the method invocation.<br>
>> > void warnUseInUnknownState(StringRef MethodName, StringRef<br>
>> > VariableName,<br>
>> > SourceLocation Loc) {<br>
>><br>
>> Why are all of the comments being removed?<br>
>><br>
><br>
> These comments are redundent, as they are also present on the virtual<br>
> versions of these functions. Rather than risk them becoming unsynchronized<br>
> I'm removing the copies here and keeping the ones on the interface.<br>
<br>
</div></div>Sounds good.<br>
<div><div class="h5"><br>
><br>
>><br>
>> ><br>
>> > Index: lib/Sema/SemaDeclAttr.cpp<br>
>> > ===================================================================<br>
>> > --- lib/Sema/SemaDeclAttr.cpp<br>
>> > +++ lib/Sema/SemaDeclAttr.cpp<br>
>> > @@ -1071,6 +1071,62 @@<br>
>> ><br>
>> > Attr.getAttributeSpellingListIndex()));<br>
>> > }<br>
>> ><br>
>> > +static void handleReturnTypestateAttr(Sema &S, Decl *D,<br>
>> > + const AttributeList &Attr) {<br>
>> > +<br>
>> > + ReturnTypestateAttr::ConsumedState ReturnState;<br>
>> > +<br>
>> > + if (Attr.getParameterName()) {<br>
>> > + StringRef Param = Attr.getParameterName()->getName();<br>
>><br>
>> You'll have to rebase this since r189711 got rid of the notion of a<br>
>> distinct parameter for attributes. It shouldn't be too bad -- the<br>
>> parameter is now the first argument in the list of args.<br>
>><br>
><br>
> Already saw that and am working on it now. This will also come in handy for<br>
> a future patch I'm about to submit.<br>
><br>
>><br>
>> > +<br>
>> > + if (Param == "unknown") {<br>
>> > + ReturnState = ReturnTypestateAttr::Unknown;<br>
>> > + } else if (Param == "consumed") {<br>
>> > + ReturnState = ReturnTypestateAttr::Consumed;<br>
>> > + } else if (Param == "unconsumed") {<br>
>> > + ReturnState = ReturnTypestateAttr::Unconsumed;<br>
>> > + } else {<br>
>> > + S.Diag(Attr.getLoc(), diag::warn_unknown_consumed_state);<br>
>><br>
>> You are missing test cases for this.<br>
>><br>
>> > + return;<br>
>> > + }<br>
>> > +<br>
>> > + } else {<br>
>> > + S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments)<br>
>> > + << Attr.getName() << 1;<br>
>><br>
>> You should use checkAttributeNumArgs instead.<br>
><br>
><br>
> That wouldn't have worked with the previous version of AttributeList as if<br>
> an attribute had a getNumArgs would still return 0. With your patch I can<br>
> now use checkAttributeNumArgs.<br>
><br>
>><br>
>><br>
>> > + }<br>
>> > +<br>
>> > + if (!isa<FunctionDecl>(D)) {<br>
>> > + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<<br>
>> > + Attr.getName() << ExpectedFunction;<br>
>> > + return;<br>
>> > + }<br>
>> > +<br>
>> > + // FIXME: This check is currently being done in the analysis. It can<br>
>> > be<br>
>> > + // enabled here only after the parser propagates attributes at<br>
>> > + // template specialization definition, not declaration.<br>
>> > + //~ QualType ReturnType;<br>
>> > + //~<br>
>> > + //~ if (const CXXConstructorDecl *Constructor =<br>
>> > dyn_cast<CXXConstructorDecl>(D)) {<br>
>> > + //~ ReturnType =<br>
>> > Constructor->getThisType(S.getASTContext())->getPointeeType();<br>
>> > + //~<br>
>> > + //~ } else {<br>
>> > + //~<br>
>> > + //~ ReturnType = cast<FunctionDecl>(D)->getCallResultType();<br>
>> > + //~ }<br>
>> > + //~<br>
>> > + //~ const CXXRecordDecl *RD = ReturnType->getAsCXXRecordDecl();<br>
>> > + //~<br>
>> > + //~ if (!RD || !RD->hasAttr<ConsumableAttr>()) {<br>
>> > + //~ S.Diag(Attr.getLoc(),<br>
>> > diag::warn_return_state_for_unconsumable_type) <<<br>
>> > + //~ ReturnType.getAsString();<br>
>> > + //~ return;<br>
>> > + //~ }<br>
>><br>
>> What is with the ~ at the start of the comments?<br>
>><br>
><br>
> These are from my IDE so it knows which comments it should remove via the<br>
> keyboard shortcut. I can remove them, or comment out this block with /* */<br>
> comments.<br>
<br>
</div></div>Thanks!<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>> > +<br>
>> > + D->addAttr(::new (S.Context)<br>
>> > + ReturnTypestateAttr(Attr.getRange(), S.Context,<br>
>> > ReturnState,<br>
>> > +<br>
>> > Attr.getAttributeSpellingListIndex()));<br>
>> > +}<br>
>> > +<br>
>> > static void handleExtVectorTypeAttr(Sema &S, Scope *scope, Decl *D,<br>
>> > const AttributeList &Attr) {<br>
>> > TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D);<br>
>> > @@ -5052,6 +5108,9 @@<br>
>> > case AttributeList::AT_TestsUnconsumed:<br>
>> > handleTestsUnconsumedAttr(S, D, Attr);<br>
>> > break;<br>
>> > + case AttributeList::AT_ReturnTypestate:<br>
>> > + handleReturnTypestateAttr(S, D, Attr);<br>
>> > + break;<br>
>> ><br>
>> > // Type safety attributes.<br>
>> > case AttributeList::AT_ArgumentWithTypeTag:<br>
>> > Index: test/SemaCXX/warn-consumed-analysis-strict.cpp<br>
>> > ===================================================================<br>
>> > --- test/SemaCXX/warn-consumed-analysis-strict.cpp<br>
>> > +++ test/SemaCXX/warn-consumed-analysis-strict.cpp<br>
>> > @@ -3,6 +3,7 @@<br>
>> > #define CALLABLE_WHEN_UNCONSUMED __attribute__<br>
>> > ((callable_when_unconsumed))<br>
>> > #define CONSUMABLE __attribute__ ((consumable))<br>
>> > #define CONSUMES __attribute__ ((consumes))<br>
>> > +#define RETURN_TYPESTATE(State) __attribute__<br>
>> > ((return_typestate(State)))<br>
>> > #define TESTS_UNCONSUMED __attribute__ ((tests_unconsumed))<br>
>> ><br>
>> > #define TEST_VAR(Var) Var.isValid()<br>
>> > @@ -15,9 +16,10 @@<br>
>> ><br>
>> > public:<br>
>> > ConsumableClass();<br>
>> > - ConsumableClass(T val);<br>
>> > - ConsumableClass(ConsumableClass<T> &other);<br>
>> > - ConsumableClass(ConsumableClass<T> &&other);<br>
>> > + ConsumableClass(nullptr_t p) RETURN_TYPESTATE(consumed);<br>
>> > + ConsumableClass(T val) RETURN_TYPESTATE(unconsumed);<br>
>> > + ConsumableClass(ConsumableClass<T> &other)<br>
>> > RETURN_TYPESTATE(unconsumed);<br>
>> > + ConsumableClass(ConsumableClass<T> &&other)<br>
>> > RETURN_TYPESTATE(unconsumed);<br>
>> ><br>
>> > ConsumableClass<T>& operator=(ConsumableClass<T> &other);<br>
>> > ConsumableClass<T>& operator=(ConsumableClass<T> &&other);<br>
>> > Index: test/SemaCXX/warn-consumed-analysis.cpp<br>
>> > ===================================================================<br>
>> > --- test/SemaCXX/warn-consumed-analysis.cpp<br>
>> > +++ test/SemaCXX/warn-consumed-analysis.cpp<br>
>> > @@ -3,6 +3,7 @@<br>
>> > #define CALLABLE_WHEN_UNCONSUMED __attribute__<br>
>> > ((callable_when_unconsumed))<br>
>> > #define CONSUMABLE __attribute__ ((consumable))<br>
>> > #define CONSUMES __attribute__ ((consumes))<br>
>> > +#define RETURN_TYPESTATE(State) __attribute__<br>
>> > ((return_typestate(State)))<br>
>> > #define TESTS_UNCONSUMED __attribute__ ((tests_unconsumed))<br>
>> ><br>
>> > typedef decltype(nullptr) nullptr_t;<br>
>> > @@ -13,10 +14,10 @@<br>
>> ><br>
>> > public:<br>
>> > ConsumableClass();<br>
>> > - ConsumableClass(nullptr_t p) CONSUMES;<br>
>> > - ConsumableClass(T val);<br>
>> > - ConsumableClass(ConsumableClass<T> &other);<br>
>> > - ConsumableClass(ConsumableClass<T> &&other);<br>
>> > + ConsumableClass(nullptr_t p) RETURN_TYPESTATE(consumed);<br>
>> > + ConsumableClass(T val) RETURN_TYPESTATE(unconsumed);<br>
>> > + ConsumableClass(ConsumableClass<T> &other)<br>
>> > RETURN_TYPESTATE(unconsumed);<br>
>> > + ConsumableClass(ConsumableClass<T> &&other)<br>
>> > RETURN_TYPESTATE(unconsumed);<br>
>> ><br>
>> > ConsumableClass<T>& operator=(ConsumableClass<T> &other);<br>
>> > ConsumableClass<T>& operator=(ConsumableClass<T> &&other);<br>
>> > @@ -48,6 +49,16 @@<br>
>> ><br>
>> > void baf3(ConsumableClass<int> &&var);<br>
>> ><br>
>> > +ConsumableClass<int> returnsUnconsumed() RETURN_TYPESTATE(unconsumed);<br>
>> > +ConsumableClass<int> returnsUnconsumed() {<br>
>> > + return ConsumableClass<int>(); // expected-warning {{return value not<br>
>> > in expected state}}<br>
>> > +}<br>
>> > +<br>
>> > +ConsumableClass<int> returnsConsumed() RETURN_TYPESTATE(consumed);<br>
>> > +ConsumableClass<int> returnsConsumed() {<br>
>> > + return ConsumableClass<int>();<br>
>> > +}<br>
>> > +<br>
>> > void testInitialization() {<br>
>> > ConsumableClass<int> var0;<br>
>> > ConsumableClass<int> var1 = ConsumableClass<int>();<br>
>> > @@ -253,6 +264,16 @@<br>
>> > *var; // expected-warning {{invocation of method 'operator*' on<br>
>> > object 'var' while it is in the 'consumed' state}}<br>
>> > }<br>
>> ><br>
>> > +void testReturnStates() {<br>
>> > + ConsumableClass<int> var;<br>
>> > +<br>
>> > + var = returnsUnconsumed();<br>
>> > + *var;<br>
>> > +<br>
>> > + var = returnsConsumed();<br>
>> > + *var; // expected-warning {{invocation of method 'operator*' on<br>
>> > object 'var' while it is in the 'consumed' state}}<br>
>> > +}<br>
>> > +<br>
>> > void testMoveAsignmentish() {<br>
>> > ConsumableClass<int> var0;<br>
>> > ConsumableClass<long> var1(42);<br>
>> > Index: test/SemaCXX/warn-consumed-parsing.cpp<br>
>> > ===================================================================<br>
>> > --- test/SemaCXX/warn-consumed-parsing.cpp<br>
>> > +++ test/SemaCXX/warn-consumed-parsing.cpp<br>
>> > @@ -1,9 +1,10 @@<br>
>> > // RUN: %clang_cc1 -fsyntax-only -verify -Wconsumed -std=c++11 %s<br>
>> ><br>
>> > -#define CONSUMABLE __attribute__ ((consumable))<br>
>> > -#define CONSUMES __attribute__ ((consumes))<br>
>> > -#define TESTS_UNCONSUMED __attribute__ ((tests_unconsumed))<br>
>> > -#define CALLABLE_WHEN_UNCONSUMED __attribute__<br>
>> > ((callable_when_unconsumed))<br>
>> > +#define CALLABLE_WHEN_UNCONSUMED __attribute__<br>
>> > ((callable_when_unconsumed))<br>
>> > +#define CONSUMABLE __attribute__ ((consumable))<br>
>> > +#define CONSUMES __attribute__ ((consumes))<br>
>> > +#define RETURN_TYPESTATE(State) __attribute__<br>
>> > ((return_typestate(State)))<br>
>> > +#define TESTS_UNCONSUMED __attribute__ ((tests_unconsumed))<br>
>> ><br>
>> > class AttrTester0 {<br>
>> > void Consumes() __attribute__ ((consumes(42))); //<br>
>> > expected-error {{attribute takes no arguments}}<br>
>> > @@ -16,6 +17,7 @@<br>
>> > int var1 TESTS_UNCONSUMED; // expected-warning {{'tests_unconsumed'<br>
>> > attribute only applies to methods}}<br>
>> > int var2 CALLABLE_WHEN_UNCONSUMED; // expected-warning<br>
>> > {{'callable_when_unconsumed' attribute only applies to methods}}<br>
>> > int var3 CONSUMABLE; // expected-warning {{'consumable' attribute only<br>
>> > applies to classes}}<br>
>> > +int var4 RETURN_TYPESTATE(consumed); // expected-warning<br>
>> > {{'return_typestate' attribute only applies to functions}}<br>
>> ><br>
>> > void function0() CONSUMES; // expected-warning {{'consumes' attribute<br>
>> > only applies to methods}}<br>
>> > void function1() TESTS_UNCONSUMED; // expected-warning<br>
>> > {{'tests_unconsumed' attribute only applies to methods}}<br>
>> > @@ -29,7 +31,13 @@<br>
>> > };<br>
>> ><br>
>> > class AttrTester2 {<br>
>> > - void callableWhenUnconsumed() CALLABLE_WHEN_UNCONSUMED; //<br>
>> > expected-warning {{consumed analysis attribute is attached to class<br>
>> > 'AttrTester2' which isn't marked as consumable}}<br>
>> > - void consumes() CONSUMES; // expected-warning<br>
>> > {{consumed analysis attribute is attached to class 'AttrTester2' which isn't<br>
>> > marked as consumable}}<br>
>> > - bool testsUnconsumed() TESTS_UNCONSUMED; // expected-warning<br>
>> > {{consumed analysis attribute is attached to class 'AttrTester2' which isn't<br>
>> > marked as consumable}}<br>
>> > + void callableWhenUnconsumed() CALLABLE_WHEN_UNCONSUMED; //<br>
>> > expected-warning {{consumed analysis attribute is attached to member of<br>
>> > class 'AttrTester2' which isn't marked as consumable}}<br>
>> > + void consumes() CONSUMES; // expected-warning<br>
>> > {{consumed analysis attribute is attached to member of class 'AttrTester2'<br>
>> > which isn't marked as consumable}}<br>
>> > + bool testsUnconsumed() TESTS_UNCONSUMED; // expected-warning<br>
>> > {{consumed analysis attribute is attached to member of class 'AttrTester2'<br>
>> > which isn't marked as consumable}}<br>
>> > };<br>
>> > +<br>
>> > +// FIXME: This can be re-added once the attribute propagation for<br>
>> > template<br>
>> > +// specializations is fixed.<br>
>> > +#if 0<br>
>> > +int returnStateFunction() RETURN_TYPESTATE(consumed); //<br>
>> > expected-warning {{return state set for an unconsumable type 'int'}}<br>
>> > +#endif<br>
>> ><br>
>><br>
>> ~Aaron<br>
><br>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">~Aaron<br>
</font></span></blockquote></div><br></div></div>