[cfe-commits] [Patch] Extend -Wloop-analysis to while and do-while loops

[Richard Trieu]

On Thu, Jun 7, 2012 at 9:21 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Jun 7, 2012, at 6:19 PM, Richard Trieu wrote:
>
> > This patch extends the loops checks that originally only had checked
> for-loops to also check while and do-while loops.  Changes are:
> >
> > - Add a bit to VarDecl to keep track if a variable has been passed by
> reference (but not const reference) or had its address taken.
>
> This particular bit should be separated from the rest of the patch,
> because it's generally useful on its own and will need a bit more
> scrutiny/validation/testing. In particular, this
>
> Index: include/clang/AST/Expr.h
> ===================================================================
> --- include/clang/AST/Expr.h    (revision 158039)
> +++ include/clang/AST/Expr.h    (working copy)
> @@ -1545,7 +1545,12 @@
>            (input->isInstantiationDependent() ||
>             type->isInstantiationDependentType()),
>            input->containsUnexpandedParameterPack()),
> -      Opc(opc), Loc(l), Val(input) {}
> +      Opc(opc), Loc(l), Val(input) {
> +    if (opc == UO_AddrOf)
> +      if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(input->IgnoreParens()))
> +        if (VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl()))
> +          VD->setReference(true);
> +  }
>
> is not something we should do in AST. Sema should be in charge of marking
> the 'address taken' bit, and should do so for any of the
> constructs---implicit or explicit---that take the address of a variable.
> And if we're going to add this AddressTaken bit, we need to find a good way
> to validate that it is correct, because it could be useful elsewhere (e.g.,
> in IR generation).
>
> I'll be splitting this patch up soon and finding a better location
checking the address of part.  I didn't think about other code being
interested in this bit as well.

>
> > - If the bit above is set on a variable, do not emit the warning.  Also,
> don't emit the warning for parameter variables in while loops.
> > - Don't warn on variable declared inside a conditional.
> > - Don't warn when there is a function call to a noreturn function inside
> the loop.
>
> @@ -1196,7 +1149,6 @@
>   class DeclMatcher : public EvaluatedExprVisitor<DeclMatcher> {
>     llvm::SmallPtrSet<VarDecl*, 8> &Decls;
>     bool FoundDecl;
> -    //bool EvalDecl;
>
>  public:
>   typedef EvaluatedExprVisitor<DeclMatcher> Inherited;
>
> Orphan declaration here?
>
Forgot to clean that up when I submitted the original patch.  Removed in
r158225.

>
> The rest (= the parts that don't introduce, set, or use the new
> 'HasReference' bit) looks okay.
>

Thanks for the fast review.  I think it's best to hold off committing until
both parts are ready.  The 'HasReference' bit helps with a common code
pattern in threaded code.

>
>        - Doug
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120608/e8e03974/attachment.html>