[cfe-commits] r67569 - in /cfe/trunk: include/clang/Basic/DiagnosticParseKinds.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Parse/Action.h include/clang/Parse/Parser.h lib/Parse/ParsePragma.cpp lib/Parse/ParsePragma.h lib/Parse/Parser.cpp lib/Sema/Sema.h lib/Sema/SemaAttr.cpp test/Sema/pragma-unused.c

[Ted Kremenek]

Thanks Doug.  Your suggested fixes would definitely improve the code.   
I'll go ahead and implement them.

On Mar 23, 2009, at 3:42 PM, Douglas Gregor wrote:

>
> On Mar 23, 2009, at 3:28 PM, Ted Kremenek wrote:
>
>> Author: kremenek
>> Date: Mon Mar 23 17:28:25 2009
>> New Revision: 67569
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=67569&view=rev
>> Log:
>> Implement '#pragma unused'.
>
> Cool. Some comments below.
>
>> Modified: cfe/trunk/include/clang/Parse/Action.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Action.h?rev=67569&r1=67568&r2=67569&view=diff
>>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> = 
>> =====================================================================
>> --- cfe/trunk/include/clang/Parse/Action.h (original)
>> +++ cfe/trunk/include/clang/Parse/Action.h Mon Mar 23 17:28:25 2009
>> @@ -1483,6 +1483,14 @@
>>                               SourceLocation RParenLoc) {
>>    return;
>>  }
>> +
>> +  /// ActOnPragmaPack - Called on well formed #pragma pack(...).
>> +  virtual void ActOnPragmaUnused(ExprTy **Exprs, unsigned NumExprs,
>> +                                 SourceLocation PragmaLoc,
>> +                                 SourceLocation LParenLoc,
>> +                                 SourceLocation RParenLoc) {
>> +    return;
>> +  }
>
> The comment here refers to #pragma pack, not #pragma unused.
>
>
>> +// #pragma unused(identifier)
>> +void PragmaUnusedHandler::HandlePragma(Preprocessor &PP, Token  
>> &UnusedTok) {
>> +  // FIXME: Should we be expanding macros here? My guess is no.
>> +  SourceLocation UnusedLoc = UnusedTok.getLocation();
>> +
>> +  // Lex the left '('.
>> +  Token Tok;
>> +  PP.Lex(Tok);
>> +  if (Tok.isNot(tok::l_paren)) {
>> +    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen)  
>> << "unused";
>> +    return;
>> +  }
>> +  SourceLocation LParenLoc = Tok.getLocation();
>> +
>> +  // Lex the declaration reference(s).
>> +  llvm::SmallVector<Action::ExprTy*, 5> Ex;
>> +  SourceLocation RParenLoc;
>> +  bool LexID = true;
>> +
>> +  while (true) {
>> +    PP.Lex(Tok);
>> +
>> +    if (LexID) {
>> +      if (Tok.is(tok::identifier)) {
>> +        Action::OwningExprResult Name =
>> +          Actions.ActOnIdentifierExpr(parser.CurScope,  
>> Tok.getLocation(),
>> +                                      *Tok.getIdentifierInfo(),  
>> false);
>> +
>> +        if (Name.isInvalid()) {
>> +          if (!Ex.empty())
>> +            Action::MultiExprArg Release(Actions, &Ex[0],  
>> Ex.size());
>> +          return;
>> +        }
>> +
>> +        Ex.push_back(Name.release());
>> +        LexID = false;
>> +        continue;
>> +      }
>> +
>> +      // Illegal token! Release the parsed expressions (if any)  
>> and emit
>> +      // a warning.
>> +      if (!Ex.empty())
>> +        Action::MultiExprArg Release(Actions, &Ex[0], Ex.size());
>> +
>> +      PP.Diag(Tok.getLocation(),  
>> diag::warn_pragma_unused_expected_var);
>> +      return;
>> +    }
>> +
>> +    // We are execting a ')' or a ','.
>> +    if (Tok.is(tok::comma)) {
>> +      LexID = true;
>> +      continue;
>> +    }
>> +
>> +    if (Tok.is(tok::r_paren)) {
>> +      RParenLoc = Tok.getLocation();
>> +      break;
>> +    }
>> +
>> +    // Illegal token! Release the parsed expressions (if any) and  
>> emit
>> +    // a warning.
>> +    if (!Ex.empty())
>> +      Action::MultiExprArg Release(Actions, &Ex[0], Ex.size());
>> +
>> +    PP.Diag(Tok.getLocation(),  
>> diag::warn_pragma_unused_expected_punc);
>> +    return;
>> +  }
>> +
>> +  // Verify that we have a location for the right parenthesis.
>> +  assert(RParenLoc.isValid() && "Valid '#pragma unused' must have  
>> ')'");
>> +  assert(!Ex.empty() && "Valid '#pragma unused' must have  
>> arguments");
>> +
>> +  // Perform the action to handle the pragma.
>> +  Actions.ActOnPragmaUnused(&Ex[0], Ex.size(), UnusedLoc,  
>> LParenLoc, RParenLoc);
>> +}
>
> We're going through some extra work here. For each name, we call  
> ActOnIdentifierExpr to build a DeclRefExpr, then we have to reverse- 
> engineer the DeclRefExpr in ActOnPragmaUnused. Perhaps it would be  
> easier to pass arrays of identifiers and locations into  
> ActOnPragmaUnused, and skip the interim representation as a  
> DeclRefExpr? ActOnPragmaUnused will, of course, need to receive the  
> scope so that it can perform lookup for those identifiers.
>
>
>> Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=67569&r1=67568&r2=67569&view=diff
>>
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- cfe/trunk/lib/Sema/SemaAttr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaAttr.cpp Mon Mar 23 17:28:25 2009
>> @@ -170,3 +170,41 @@
>>  }
>> }
>>
>> +void Sema::ActOnPragmaUnused(ExprTy **Exprs, unsigned NumExprs,
>> +                             SourceLocation PragmaLoc,
>> +                             SourceLocation LParenLoc,
>> +                             SourceLocation RParenLoc) {
>> +
>> +  // Verify that all of the expressions are valid before
>> +  // modifying the attributes of any referenced decl.
>> +  Expr *ErrorExpr = 0;
>> +
>> +  for (unsigned i = 0; i < NumExprs; ++i) {
>> +    Expr *Ex = (Expr*) Exprs[i];
>> +    if (!isa<DeclRefExpr>(Ex)) {
>> +      ErrorExpr = Ex;
>> +      break;
>> +    }
>> +
>> +    Decl *d = cast<DeclRefExpr>(Ex)->getDecl();;
>> +
>> +    if (!isa<VarDecl>(d) || !cast<VarDecl>(d)->hasLocalStorage()) {
>> +      ErrorExpr = Ex;
>> +      break;
>> +    }
>> +  }
>> +
>> +  // Delete the expressions if we encountered any error.
>> +  if (ErrorExpr) {
>> +    Diag(ErrorExpr->getLocStart(),  
>> diag::warn_pragma_unused_expected_localvar);
>> +    for (unsigned i = 0; i < NumExprs; ++i)
>> +      ((Expr*) Exprs[i])->Destroy(Context);
>> +    return;
>> +  }
>> +
>> +  // Otherwise, add the 'unused' attribute to each referenced  
>> declaration.
>> +  for (unsigned i = 0; i < NumExprs; ++i) {
>> +    DeclRefExpr *DR = (DeclRefExpr*) Exprs[i];
>> +    DR->getDecl()->addAttr(::new (Context) UnusedAttr());
>> +  }
>> +}
>
> Much of this code could be simplified if we just performed the  
> lookup of these variables here. Also, aren't we leaking the  
> DeclRefExprs in the case where the #pragma unused is well-formed?
>
> 	- Doug