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