<div dir="ltr"><div>Responses are bellow. Thanks for the review.<br><br></div>- Chris<br><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 19, 2013 at 12:28 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Observations interspersed below.<br>
<div><div class="h5"><br>
On Mon, Aug 19, 2013 at 2:57 PM, Christian Wailes<br>
<<a href="mailto:chriswailes@google.com">chriswailes@google.com</a>> wrote:<br>
> chriswailes added you to the CC list for the revision "Fixes and features for Consumed analysis".<br>
><br>
> Hi delesley, dblaikie,<br>
><br>
> This patch provides fixes to the code based on feedback, as well as a couple of new features:<br>
><br>
> * The same functionality is now supported for both CXXOperatorCallExprs and CXXMemberCallExprs.<br>
> * Factored out some code in the StmtVisitor.<br>
> * Removed unnecessary asserts from SemaDeclAttr.cpp<br>
> * Remove variables from the state map when their destructors are encountered.<br>
> * Re-use an existing warning for misplaced attributes.<br>
> * Started adding documentation for the consumed analysis attributes.<br>
><br>
> <a href="http://llvm-reviews.chandlerc.com/D1384" target="_blank">http://llvm-reviews.chandlerc.com/D1384</a><br>
><br>
> Files:<br>
> docs/LanguageExtensions.rst<br>
> include/clang/Analysis/Analyses/Consumed.h<br>
> include/clang/Basic/Attr.td<br>
> include/clang/Basic/DiagnosticSemaKinds.td<br>
> lib/Analysis/Consumed.cpp<br>
> lib/Sema/AnalysisBasedWarnings.cpp<br>
> lib/Sema/SemaDeclAttr.cpp<br>
> test/SemaCXX/warn-consumed-analysis.cpp<br>
><br>
</div></div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
> Index: docs/LanguageExtensions.rst<br>
> ===================================================================<br>
> --- docs/LanguageExtensions.rst<br>
> +++ docs/LanguageExtensions.rst<br>
> @@ -2022,6 +2022,33 @@<br>
> locks. Arguments must be lockable type, and there must be at least one<br>
> argument.<br>
><br>
> +Consumed Annotation Checking<br>
> +============================<br>
> +<br>
> +Clang supports additional attributes for checking basic resource management<br>
> +properties, specifically for unique objects that have a single owning reference.<br>
> +The following attributes are currently supported, although **the implementation<br>
> +for these annotations is currently in development and are subject to change.**<br>
> +<br>
> +``consumes``<br>
> +------------<br>
> +<br>
> +Use ``__attribute__((consumes))`` on a method that transitions an object into<br>
> +the consumed state.<br>
> +<br>
> +``callable_when_unconsumed``<br>
> +----------------------------<br>
> +<br>
> +Use ``__attribute__((callable_when_unconsumed))`` to indicate that a method may<br>
> +only be called when the object is not in the consumed state.<br>
> +<br>
> +``tests_unconsumed``<br>
> +--------------------<br>
> +<br>
> +Use `__attribute__((tests_unconsumed))`` to indicate that a method returns true<br>
> +if the object is in the unconsumed state.<br>
> +<br>
> +<br>
> Type Safety Checking<br>
> ====================<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>
> @@ -118,6 +118,9 @@<br>
><br>
> /// \brief Set the consumed state of a given variable.<br>
> void setState(const VarDecl *Var, ConsumedState State);<br>
> +<br>
> + /// \brief Remove the variable from our state map.<br>
> + void remove(const VarDecl *Var);<br>
> };<br>
><br>
> class ConsumedBlockInfo {<br>
> @@ -188,7 +191,7 @@<br>
> };<br>
><br>
> /// \brief Check to see if a function tests an object's validity.<br>
> - bool isTestingFunction(const CXXMethodDecl *MethodDecl);<br>
> + bool isTestingFunction(const FunctionDecl *FunDecl);<br>
><br>
> }} // end namespace clang::consumed<br>
><br>
> Index: include/clang/Basic/Attr.td<br>
> ===================================================================<br>
> --- include/clang/Basic/Attr.td<br>
> +++ include/clang/Basic/Attr.td<br>
> @@ -932,7 +932,7 @@<br>
><br>
> def Consumes : InheritableAttr {<br>
> let Spellings = [GNU<"consumes">];<br>
> - let Subjects = [CXXMethod];<br>
> + let Subjects = [CXXMethod, CXXConstructor];<br>
> }<br>
><br>
> def TestsConsumed : InheritableAttr {<br>
> Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
> ===================================================================<br>
> --- include/clang/Basic/DiagnosticSemaKinds.td<br>
> +++ include/clang/Basic/DiagnosticSemaKinds.td<br>
> @@ -2189,9 +2189,6 @@<br>
> def warn_use_of_temp_while_consumed : Warning<<br>
> "invocation of method '%0' on a temporary object while it is in the "<br>
> "'consumed' state">, InGroup<Consumed>, DefaultIgnore;<br>
> -def warn_uniqueness_attribute_wrong_decl_type : Warning<<br>
> - "%0 attribute only applies to methods">,<br>
> - InGroup<Consumed>, DefaultIgnore;<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>
> @@ -30,7 +30,6 @@<br>
> #include "llvm/ADT/SmallVector.h"<br>
> #include "llvm/Support/raw_ostream.h"<br>
><br>
> -// TODO: Add support for methods with CallableWhenUnconsumed.<br>
> // TODO: Mark variables as Unknown going into while- or for-loops only if they<br>
> // are referenced inside that block. (Deferred)<br>
> // TODO: Add a method(s) to identify which method calls perform what state<br>
> @@ -91,9 +90,9 @@<br>
> StateOrVar.Var = Var;<br>
> }<br>
><br>
> - ConsumedState getState() { return StateOrVar.State; };<br>
> + ConsumedState getState() const { return StateOrVar.State; };<br>
<br>
There's a spurious semi colon after the curly brace.<br>
<br></blockquote><div> </div><div>Fixed<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> - const VarDecl * getVar() { return IsVar ? StateOrVar.Var : NULL; };<br>
> + const VarDecl * getVar() const { return IsVar ? StateOrVar.Var : NULL; };<br>
<br>
Same here.<br>
<br></blockquote><div> </div><div>Fixed<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> };<br>
><br>
> typedef llvm::DenseMap<const Stmt *, PropagationInfo> MapType;<br>
> @@ -105,6 +104,9 @@<br>
> ConsumedStateMap *StateMap;<br>
> MapType PropagationMap;<br>
><br>
> + void checkCallability(const PropagationInfo &PState,<br>
> + const FunctionDecl *FunDecl,<br>
> + const CallExpr *Call);<br>
<br>
Formatting issues with the alignment of the last two parameters.<br>
<br></blockquote><div> </div><div>I don't see this in my editor, but I'll check to make sure the next patch I submit doesn't have it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> void forwardInfo(const Stmt *From, const Stmt *To);<br>
> bool isLikeMoveAssignment(const CXXMethodDecl *MethodDecl);<br>
><br>
> @@ -128,12 +130,58 @@<br>
> ConsumedStmtVisitor(AnalysisDeclContext &AC, ConsumedAnalyzer &Analyzer,<br>
> ConsumedStateMap *StateMap)<br>
> : AC(AC), Analyzer(Analyzer), StateMap(StateMap) {}<br>
> -<br>
> +<br>
<br>
Whitespace change?<br>
<br></blockquote><div> </div><div>Fixed<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> void reset() {<br>
> PropagationMap.clear();<br>
> }<br>
> };<br>
><br>
> +// TODO: When we support CallableWhenConsumed this will have to check for<br>
> +// the different attributes and change the behavior bellow. (Deferred)<br>
> +void ConsumedStmtVisitor::checkCallability(const PropagationInfo &PState,<br>
> + const FunctionDecl *FunDecl,<br>
> + const CallExpr *Call) {<br>
<br>
More formatting.<br>
<br></blockquote><div> </div><div>Again, not present in my editor, but I'll check the next patch.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> + if (!FunDecl->hasAttr<CallableWhenUnconsumedAttr>()) return;<br>
> +<br>
> + if (PState.IsVar) {<br>
> + const VarDecl *Var = PState.getVar();<br>
> +<br>
> + switch (StateMap->getState(Var)) {<br>
> + case CS_Consumed:<br>
> + Analyzer.WarningsHandler.warnUseWhileConsumed(<br>
> + FunDecl->getNameAsString(), Var->getNameAsString(),<br>
> + Call->getExprLoc());<br>
> + break;<br>
> +<br>
> + case CS_Unknown:<br>
> + Analyzer.WarningsHandler.warnUseInUnknownState(<br>
> + FunDecl->getNameAsString(), Var->getNameAsString(),<br>
> + Call->getExprLoc());<br>
> + break;<br>
> +<br>
> + default:<br>
> + break;<br>
<br>
Should the default be unreachable?<br>
<br></blockquote><div> </div><div>No, there are other cases besides Consumed and Unknown. I just don't want to do anything for those cases.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + }<br>
> +<br>
> + } else {<br>
> + switch (PState.getState()) {<br>
> + case CS_Consumed:<br>
> + Analyzer.WarningsHandler.warnUseOfTempWhileConsumed(<br>
> + FunDecl->getNameAsString(), Call->getExprLoc());<br>
> + break;<br>
> +<br>
> + case CS_Unknown:<br>
> + Analyzer.WarningsHandler.warnUseOfTempInUnknownState(<br>
> + FunDecl->getNameAsString(), Call->getExprLoc());<br>
> + break;<br>
> +<br>
> + default:<br>
> + break;<br>
<br>
Same here.<br>
<br></blockquote><div> </div><div>Same as above.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + }<br>
> + }<br>
> +}<br>
> +<br>
> void ConsumedStmtVisitor::forwardInfo(const Stmt *From, const Stmt *To) {<br>
> InfoEntry Entry = PropagationMap.find(From);<br>
><br>
> @@ -278,14 +326,16 @@<br>
><br>
> if (Entry != PropagationMap.end()) {<br>
> PropagationInfo PState = Entry->second;<br>
> - if (!PState.IsVar) return;<br>
> + const CXXMethodDecl *MethodDecl = Call->getMethodDecl();<br>
><br>
> - const CXXMethodDecl *Method = Call->getMethodDecl();<br>
> + checkCallability(PState, MethodDecl, Call);<br>
><br>
> - if (Method->hasAttr<ConsumesAttr>())<br>
> - StateMap->setState(PState.getVar(), consumed::CS_Consumed);<br>
> - else if (!Method->isConst())<br>
> - StateMap->setState(PState.getVar(), consumed::CS_Unknown);<br>
> + if (PState.IsVar) {<br>
> + if (MethodDecl->hasAttr<ConsumesAttr>())<br>
> + StateMap->setState(PState.getVar(), consumed::CS_Consumed);<br>
> + else if (!MethodDecl->isConst())<br>
> + StateMap->setState(PState.getVar(), consumed::CS_Unknown);<br>
> + }<br>
> }<br>
> }<br>
><br>
> @@ -374,58 +424,22 @@<br>
> InfoEntry Entry = PropagationMap.find(Call->getArg(0));<br>
><br>
> if (Entry != PropagationMap.end()) {<br>
> -<br>
> PropagationInfo PState = Entry->second;<br>
><br>
> - // TODO: When we support CallableWhenConsumed this will have to check for<br>
> - // the different attributes and change the behavior bellow.<br>
> - // (Deferred)<br>
> - if (FunDecl->hasAttr<CallableWhenUnconsumedAttr>()) {<br>
> - if (PState.IsVar) {<br>
> - const VarDecl *Var = PState.getVar();<br>
> -<br>
> - switch (StateMap->getState(Var)) {<br>
> - case CS_Consumed:<br>
> - Analyzer.WarningsHandler.warnUseWhileConsumed(<br>
> - FunDecl->getNameAsString(), Var->getNameAsString(),<br>
> - Call->getExprLoc());<br>
> - break;<br>
> -<br>
> - case CS_Unknown:<br>
> - Analyzer.WarningsHandler.warnUseInUnknownState(<br>
> - FunDecl->getNameAsString(), Var->getNameAsString(),<br>
> - Call->getExprLoc());<br>
> - break;<br>
> -<br>
> - default:<br>
> - break;<br>
> - }<br>
> -<br>
> - } else {<br>
> - switch (PState.getState()) {<br>
> - case CS_Consumed:<br>
> - Analyzer.WarningsHandler.warnUseOfTempWhileConsumed(<br>
> - FunDecl->getNameAsString(), Call->getExprLoc());<br>
> - break;<br>
> + checkCallability(PState, FunDecl, Call);<br>
> +<br>
> + if (PState.IsVar) {<br>
> + if (FunDecl->hasAttr<ConsumesAttr>()) {<br>
> + // Handle consuming operators.<br>
> + StateMap->setState(PState.getVar(), consumed::CS_Consumed);<br>
> + } else if (const CXXMethodDecl *MethodDecl =<br>
> + dyn_cast_or_null<CXXMethodDecl>(FunDecl)) {<br>
><br>
> - case CS_Unknown:<br>
> - Analyzer.WarningsHandler.warnUseOfTempInUnknownState(<br>
> - FunDecl->getNameAsString(), Call->getExprLoc());<br>
> - break;<br>
> -<br>
> - default:<br>
> - break;<br>
> - }<br>
> + // Handle non-constant member operators.<br>
> + if (!MethodDecl->isConst())<br>
> + StateMap->setState(PState.getVar(), consumed::CS_Unknown);<br>
> }<br>
> }<br>
> -<br>
> - // Handle non-constant member operators.<br>
> - if (const CXXMethodDecl *MethodDecl =<br>
> - dyn_cast_or_null<CXXMethodDecl>(FunDecl)) {<br>
> -<br>
> - if (!MethodDecl->isConst() && PState.IsVar)<br>
> - StateMap->setState(PState.getVar(), consumed::CS_Unknown);<br>
> - }<br>
> }<br>
> }<br>
> }<br>
> @@ -505,10 +519,10 @@<br>
> };<br>
><br>
> bool TestedVarsVisitor::VisitCallExpr(CallExpr *Call) {<br>
> - if (const CXXMethodDecl *Method =<br>
> - dyn_cast_or_null<CXXMethodDecl>(Call->getDirectCallee())) {<br>
> + if (const FunctionDecl *FunDecl =<br>
> + dyn_cast_or_null<FunctionDecl>(Call->getDirectCallee())) {<br>
><br>
> - if (isTestingFunction(Method)) {<br>
> + if (isTestingFunction(FunDecl)) {<br>
> CurrTestLoc = Call->getExprLoc();<br>
> IsUsefulConditional = true;<br>
> return true;<br>
> @@ -521,16 +535,11 @@<br>
> }<br>
><br>
> bool TestedVarsVisitor::VisitDeclRefExpr(DeclRefExpr *DeclRef) {<br>
> - if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(DeclRef->getDecl())) {<br>
> - if (StateMap->getState(Var) != consumed::CS_None) {<br>
> + if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(DeclRef->getDecl()))<br>
> + if (StateMap->getState(Var) != consumed::CS_None)<br>
<br>
Can these if's be combined for clarity?<br>
<br></blockquote><div> </div><div>No, if you delcare a variable in an if-statement then you can't have any other claues.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Test = VarTestResult(Var, CurrTestLoc, !Invert);<br>
> - }<br>
> -<br>
> - } else {<br>
> - IsUsefulConditional = false;<br>
> - }<br>
><br>
> - return IsUsefulConditional;<br>
> + return true;<br>
> }<br>
><br>
> bool TestedVarsVisitor::VisitUnaryOperator(UnaryOperator *UnaryOp) {<br>
> @@ -637,6 +646,9 @@<br>
> Map[Var] = State;<br>
> }<br>
><br>
> +void ConsumedStateMap::remove(const VarDecl *Var) {<br>
> + Map.erase(Var);<br>
> +}<br>
><br>
> bool ConsumedAnalyzer::isConsumableType(QualType Type) {<br>
> const CXXRecordDecl *RD =<br>
> @@ -734,20 +746,24 @@<br>
><br>
> if (CurrStates == NULL)<br>
> CurrStates = BlockInfo.getInfo(CurrBlock);<br>
> -<br>
> +<br>
<br>
Whitespace only?<br>
<br></blockquote><div> </div><div>Fixed<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> ConsumedStmtVisitor Visitor(AC, *this, CurrStates);<br>
> -<br>
> +<br>
<br>
Whitespace only?<br>
<br></blockquote><div> </div><div>Fixed<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> // Visit all of the basic block's statements.<br>
> for (CFGBlock::const_iterator BI = CurrBlock->begin(),<br>
> BE = CurrBlock->end(); BI != BE; ++BI) {<br>
><br>
> - if (BI->getKind() == CFGElement::Statement)<br>
> + switch (BI->getKind()) {<br>
> + case CFGElement::Statement:<br>
> Visitor.Visit(BI->castAs<CFGStmt>().getStmt());<br>
> + break;<br>
> + case CFGElement::AutomaticObjectDtor:<br>
> + CurrStates->remove(BI->castAs<CFGAutomaticObjDtor>().getVarDecl());<br>
> + default:<br>
> + break;<br>
<br>
Should this default be unreachable?<br>
<br></blockquote><div> </div><div>Nope.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + }<br>
> }<br>
><br>
> - // TODO: Remove any variables that have reached the end of their<br>
> - // lifetimes from the state map. (Deferred)<br>
> -<br>
> if (const IfStmt *Terminator =<br>
> dyn_cast_or_null<IfStmt>(CurrBlock->getTerminator().getStmt())) {<br>
><br>
> @@ -786,8 +802,8 @@<br>
> WarningsHandler.emitDiagnostics();<br>
> }<br>
><br>
> -bool isTestingFunction(const CXXMethodDecl *Method) {<br>
> - return Method->hasAttr<TestsUnconsumedAttr>();<br>
> +bool isTestingFunction(const FunctionDecl *FunDecl) {<br>
> + return FunDecl->hasAttr<TestsUnconsumedAttr>();<br>
> }<br>
<br>
Can this function be static?<br>
<br></blockquote><div> </div><div>Done.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> }} // end namespace clang::consumed<br>
> Index: lib/Sema/AnalysisBasedWarnings.cpp<br>
> ===================================================================<br>
> --- lib/Sema/AnalysisBasedWarnings.cpp<br>
> +++ lib/Sema/AnalysisBasedWarnings.cpp<br>
> @@ -1551,10 +1551,9 @@<br>
> DefaultPolicy.enableThreadSafetyAnalysis = (unsigned)<br>
> (D.getDiagnosticLevel(diag::warn_double_lock, SourceLocation()) !=<br>
> DiagnosticsEngine::Ignored);<br>
> - DefaultPolicy.enableConsumedAnalysis =<br>
> - (unsigned)(D.getDiagnosticLevel(diag::warn_use_while_consumed,<br>
> - SourceLocation()) !=<br>
> - DiagnosticsEngine::Ignored);<br>
> + DefaultPolicy.enableConsumedAnalysis = (unsigned)<br>
> + (D.getDiagnosticLevel(diag::warn_use_while_consumed, SourceLocation()) !=<br>
> + DiagnosticsEngine::Ignored);<br>
> }<br>
><br>
> static void flushDiagnostics(Sema &S, sema::FunctionScopeInfo *fscope) {<br>
> Index: lib/Sema/SemaDeclAttr.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaDeclAttr.cpp<br>
> +++ lib/Sema/SemaDeclAttr.cpp<br>
> @@ -497,8 +497,6 @@<br>
<br>
Removing the assert(!Attr.isInvalid()); is a good change, but should<br>
be submitted separately (not as part of this patch).<br>
<br></blockquote><div> </div><div>While this can be done I would have to walk back some patches break this one out, re-apply patches, and then submit another patch. I admit I should have done a seperate patch, but at this point it would be a non-trivial amount of work for very little benefit. I would like to just let this one slide and keep this in mind for future patches.<br>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> static bool checkGuardedVarAttrCommon(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeNumArgs(S, Attr, 0))<br>
> return false;<br>
><br>
> @@ -537,8 +535,6 @@<br>
> static bool checkGuardedByAttrCommon(Sema &S, Decl *D,<br>
> const AttributeList &Attr,<br>
> Expr* &Arg) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeNumArgs(S, Attr, 1))<br>
> return false;<br>
><br>
> @@ -584,8 +580,6 @@<br>
><br>
> static bool checkLockableAttrCommon(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeNumArgs(S, Attr, 0))<br>
> return false;<br>
><br>
> @@ -618,8 +612,6 @@<br>
><br>
> static void handleNoThreadSafetyAnalysis(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeNumArgs(S, Attr, 0))<br>
> return;<br>
><br>
> @@ -635,8 +627,6 @@<br>
><br>
> static void handleNoSanitizeAddressAttr(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeNumArgs(S, Attr, 0))<br>
> return;<br>
><br>
> @@ -653,8 +643,6 @@<br>
><br>
> static void handleNoSanitizeMemory(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeNumArgs(S, Attr, 0))<br>
> return;<br>
><br>
> @@ -670,8 +658,6 @@<br>
><br>
> static void handleNoSanitizeThread(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeNumArgs(S, Attr, 0))<br>
> return;<br>
><br>
> @@ -688,8 +674,6 @@<br>
> static bool checkAcquireOrderAttrCommon(Sema &S, Decl *D,<br>
> const AttributeList &Attr,<br>
> SmallVectorImpl<Expr *> &Args) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeAtLeastNumArgs(S, Attr, 1))<br>
> return false;<br>
><br>
> @@ -749,8 +733,6 @@<br>
> static bool checkLockFunAttrCommon(Sema &S, Decl *D,<br>
> const AttributeList &Attr,<br>
> SmallVectorImpl<Expr *> &Args) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> // zero or more arguments ok<br>
><br>
> // check that the attribute is applied to a function<br>
> @@ -824,8 +806,7 @@<br>
> static bool checkTryLockFunAttrCommon(Sema &S, Decl *D,<br>
> const AttributeList &Attr,<br>
> SmallVectorImpl<Expr *> &Args) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> +<br>
<br>
Whitespace only?<br>
<br>
> if (!checkAttributeAtLeastNumArgs(S, Attr, 1))<br>
> return false;<br>
><br>
> @@ -878,8 +859,6 @@<br>
> static bool checkLocksRequiredCommon(Sema &S, Decl *D,<br>
> const AttributeList &Attr,<br>
> SmallVectorImpl<Expr *> &Args) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeAtLeastNumArgs(S, Attr, 1))<br>
> return false;<br>
><br>
> @@ -925,8 +904,6 @@<br>
><br>
> static void handleUnlockFunAttr(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> // zero or more arguments ok<br>
><br>
> if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {<br>
> @@ -948,8 +925,6 @@<br>
><br>
> static void handleLockReturnedAttr(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeNumArgs(S, Attr, 1))<br>
> return;<br>
><br>
> @@ -973,8 +948,6 @@<br>
><br>
> static void handleLocksExcludedAttr(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> -<br>
> if (!checkAttributeAtLeastNumArgs(S, Attr, 1))<br>
> return;<br>
><br>
> @@ -999,12 +972,11 @@<br>
><br>
> static void handleConsumesAttr(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> if (!checkAttributeNumArgs(S, Attr, 0)) return;<br>
><br>
> if (!(isa<CXXMethodDecl>(D) || isa<CXXConstructorDecl>(D))) {<br>
> - S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<<br>
> - Attr.getName();<br>
> + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<<br>
> + Attr.getName() << "methods";<br>
<br>
You should be using a value from AttributeDeclKind (probably<br>
ExpectedMethod) instead of a string literal. Should probably have a<br>
test to ensure this is properly caught.<br>
<br></blockquote><div><br></div><div>Switched to ExpectedMethod. What kind of tests are you proposing?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> return;<br>
> }<br>
><br>
> @@ -1015,12 +987,11 @@<br>
><br>
> static void handleCallableWhenUnconsumedAttr(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> if (!checkAttributeNumArgs(S, Attr, 0)) return;<br>
><br>
> if (!isa<CXXMethodDecl>(D)) {<br>
> - S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<<br>
> - Attr.getName();<br>
> + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<<br>
> + Attr.getName() << "methods";<br>
<br>
Same here.<br>
<br>
> return;<br>
> }<br>
><br>
> @@ -1031,12 +1002,11 @@<br>
><br>
> static void handleTestsConsumedAttr(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> if (!checkAttributeNumArgs(S, Attr, 0)) return;<br>
><br>
> if (!isa<CXXMethodDecl>(D)) {<br>
> - S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<<br>
> - Attr.getName();<br>
> + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<<br>
> + Attr.getName() << "methods";<br>
<br>
And here.<br>
<br>
> return;<br>
> }<br>
><br>
> @@ -1047,12 +1017,11 @@<br>
><br>
> static void handleTestsUnconsumedAttr(Sema &S, Decl *D,<br>
> const AttributeList &Attr) {<br>
> - assert(!Attr.isInvalid());<br>
> if (!checkAttributeNumArgs(S, Attr, 0)) return;<br>
><br>
> if (!isa<CXXMethodDecl>(D)) {<br>
> - S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<<br>
> - Attr.getName();<br>
> + S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<<br>
> + Attr.getName() << "methods";<br>
<br>
And here.<br>
<br>
> return;<br>
> }<br>
><br>
> Index: test/SemaCXX/warn-consumed-analysis.cpp<br>
> ===================================================================<br>
> --- test/SemaCXX/warn-consumed-analysis.cpp<br>
> +++ test/SemaCXX/warn-consumed-analysis.cpp<br>
> @@ -27,10 +27,13 @@<br>
> template <typename U><br>
> Bar<T>& operator=(Bar<U> &&other);<br>
><br>
> + void operator()(int a) CONSUMES;<br>
> void operator*(void) const CALLABLE_WHEN_UNCONSUMED;<br>
> + void unconsumedCall(void) const CALLABLE_WHEN_UNCONSUMED;<br>
><br>
> bool isValid(void) const TESTS_UNCONSUMED;<br>
> operator bool() const TESTS_UNCONSUMED;<br>
> + bool operator!=(nullptr_t) const TESTS_UNCONSUMED;<br>
><br>
> void constCall(void) const;<br>
> void nonconstCall(void);<br>
> @@ -98,6 +101,13 @@<br>
> } else {<br>
> *var; // expected-warning {{invocation of method 'operator*' on object 'var' while it is in the 'consumed' state}}<br>
> }<br>
> +<br>
> + if (var != nullptr) {<br>
> + // Empty<br>
> +<br>
> + } else {<br>
> + *var; // expected-warning {{invocation of method 'operator*' on object 'var' while it is in the 'consumed' state}}<br>
> + }<br>
> }<br>
><br>
> void testCallingConventions(void) {<br>
> @@ -164,6 +174,15 @@<br>
> *var; // expected-warning {{invocation of method 'operator*' on object 'var' while it is in the 'consumed' state}}<br>
> }<br>
><br>
> +void testConsumes2(void) {<br>
> + Bar<int> var(42);<br>
> +<br>
> + var.unconsumedCall();<br>
> + var(6);<br>
> +<br>
> + var.unconsumedCall(); // expected-warning {{invocation of method 'unconsumedCall' on object 'var' while it is in the 'consumed' state}}<br>
> +}<br>
> +<br>
> void testSimpleForLoop(void) {<br>
> Bar<int> var;<br>
<br>
~Aaron<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div></div></div>