<div dir="ltr"><div>I would like such a test as well. I even have a bit of code locally that does just that. It has the declaration with the annotation on it (returning an int), and then a declaration. The declaration is necessary because the analysis fires when a function's declaration is finalized. The warning is issued as expected. However, when I add a definition below the declaration in the unit test file no warning is issued. I'm not sure what could be causing the difference. If you have any ideas, I'd appreciate the help. I'll finish making my changes for your patch and upload the updated diff.<br>
<br></div>- Chris<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 2, 2013 at 5:32 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 8:12 PM, Christian Wailes <<a href="mailto:chriswailes@google.com">chriswailes@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Mon, Sep 2, 2013 at 3:54 PM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Sep 2, 2013 at 6:45 PM, Christian Wailes <<a href="mailto:chriswailes@google.com">chriswailes@google.com</a>><br>
>> 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<br>
>> >> > types.<br>
>> >> > + ///<br>
>> >> > + /// \param Loc -- The location of the attributes.<br>
>> >> > + ///<br>
>> >> > + /// \param TypeName -- The name of the unconsumable type.<br>
>> >> > + virtual void<br>
>> >> > 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<br>
>> >> > 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<br>
>> >> > 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<br>
>> >> > 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'">,<br>
>> >> > InGroup<Consumed>,<br>
>> >> > + DefaultIgnore;<br>
>> >> > +def warn_unknown_consumed_state : Warning<<br>
>> >> > + "unknown consumed analysis state">, InGroup<Consumed>,<br>
>> >> > 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<br>
>> > of a<br>
>> > nice, concise way of expressing it in the warning. This will also allow<br>
>> > me<br>
>> > to point to where the return typestate is declared.<br>
>><br>
>> 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>
><br>
><br>
> Done.<br>
><br>
>><br>
>><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<br>
>> >> > *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<br>
>> >> > *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>
>> >> > +<br>
>> >> > PropagationInfo(consumed::CS_Consumed)));<br>
>> >> > + break;<br>
>> >> > +<br>
>> >> > + case ReturnTypestateAttr::Unknown:<br>
>> >> > + PropagationMap.insert(PairType(Call,<br>
>> >> > +<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,<br>
>> >> > 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<br>
>> >> > *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 =<br>
>> >> > 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 =<br>
>> >> > 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) <<<br>
>> >> > 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<br>
>> > incorrectly<br>
>> > propagated for template specializations is preventing us from checking<br>
>> > for<br>
>> > this problem when parsing the attributes.<br>
>><br>
>> 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>
>><br>
><br>
> Can you elaborate on what you mean by an 'enabled test'? And while we could<br>
> test for non-template specialized cases in SemaDeclAttr.cpp now, I was told<br>
> to comment out that code and check for the warning in the ConsumedAnalyzer<br>
> by DeLesely.<br>
<br>
</div></div>Currently, the only test for<br>
warn_return_typestate_for_unconsumable_type is in a #if 0 block; we<br>
should have a test for that warning which isn't #if'ed out. I'm not<br>
worried about which part of the code does the analysis for the<br>
warning, just that the warning is covered by at least one test case<br>
that will run.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>> > There are some FIXMEs running<br>
>> > around telling someone to un-comment the test and move some code around<br>
>> > once<br>
>> > the issue is resolved. Specifically, this warning function will be<br>
>> > 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<br>
>> >> > incorrectly<br>
>> >> > - /// invoked.<br>
>> >> > - ///<br>
>> >> > - /// \param Loc -- The SourceLocation of the method invocation.<br>
>> >> > void warnUseOfTempWhileConsumed(StringRef MethodName,<br>
>> >> > 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<br>
>> >> > incorrectly<br>
>> >> > - /// invoked.<br>
>> >> > - ///<br>
>> >> > - /// \param Loc -- The SourceLocation of the method invocation.<br>
>> >> > void warnUseOfTempInUnknownState(StringRef MethodName,<br>
>> >> > 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<br>
>> >> > 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<br>
>> >> > 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<br>
>> > unsynchronized<br>
>> > I'm removing the copies here and keeping the ones on the interface.<br>
>><br>
>> Sounds good.<br>
>><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<br>
>> > 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(),<br>
>> >> > 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<br>
>> > if<br>
>> > an attribute had a getNumArgs would still return 0. With your patch I<br>
>> > 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<br>
>> >> > can<br>
>> >> > be<br>
>> >> > + // enabled here only after the parser propagates attributes<br>
>> >> > 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<br>
>> > the<br>
>> > keyboard shortcut. I can remove them, or comment out this block with /*<br>
>> > */<br>
>> > comments.<br>
>><br>
>> Thanks!<br>
>><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()<br>
>> >> > RETURN_TYPESTATE(unconsumed);<br>
>> >> > +ConsumableClass<int> returnsUnconsumed() {<br>
>> >> > + return ConsumableClass<int>(); // expected-warning {{return value<br>
>> >> > 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<br>
>> >> > 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'<br>
>> >> > 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'<br>
>> >> > which isn't<br>
>> >> > marked as consumable}}<br>
>> >> > - bool testsUnconsumed() TESTS_UNCONSUMED; //<br>
>> >> > expected-warning<br>
>> >> > {{consumed analysis attribute is attached to class 'AttrTester2'<br>
>> >> > which isn't<br>
>> >> > marked as consumable}}<br>
>> >> > + void callableWhenUnconsumed() CALLABLE_WHEN_UNCONSUMED; //<br>
>> >> > expected-warning {{consumed analysis attribute is attached to member<br>
>> >> > 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<br>
>> >> > 'AttrTester2'<br>
>> >> > which isn't marked as consumable}}<br>
>> >> > + bool testsUnconsumed() TESTS_UNCONSUMED; //<br>
>> >> > expected-warning<br>
>> >> > {{consumed analysis attribute is attached to member of class<br>
>> >> > '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>
>> ~Aaron<br>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">~Aaron<br>
</font></span></blockquote></div><br></div>