[cfe-commits] [PATCH] CFG automatic object destructors

[Marcin Świderski]

Thanks for looking on the patch.

W dniu 28 września 2010 03:31 użytkownik Ted Kremenek
<kremenek at apple.com>napisał:

>
> On Sep 27, 2010, at 3:55 PM, Marcin Świderski wrote:
>
> > Patch adds implicit destructors generation for objects with automatic
> storage duration. Patch is rather big, but it contains all cases (that I
> thought of), as it will be easier to review as a whole IMO. While commiting
> I can divide it to smaller chuncks.
>
> Overall this looks great.  There are a few misspelled words in the comments
> resulting from typos (e.g., 'Elememnts'), so it's probably worth running a
> spell-checker.
>
> Besides specific comments (below), I'm concerned about the impact on
> CFG-construction time when scope building is *not* enabled.  This is
> currently the common case for uses of the CFG in Sema.  I think there
> shouldn't be any noticeable penalty since most of the logic is guarded with
> a flag, but it is worth verifying.
>
> Is there any performance benchmark prepared for such cases?


> >
> > I've added some test cases. They can be later used to create regresion
> tests.
>
> For the test cases, how do you plan on testing it?  Also, since these are
> all small tests, please include them in one file (if possible).
>
> You did mention something about similar case in test folder, but I can't
currently locate your message. Some suggestions would be useful, e.g. how to
set up test for feature that is currently turned off.


> A few specific comments inline:
>
> Index: include/clang/Analysis/CFG.h
> ===================================================================
> --- include/clang/Analysis/CFG.h        (revision 114789)
> +++ include/clang/Analysis/CFG.h        (working copy)
> @@ -1159,7 +1417,7 @@
>       // No increment code.  Create a special, empty, block that is used as
> the
>       // target block for "looping back" to the start of the loop.
>       assert(Succ == EntryConditionBlock);
> -      Succ = createBlock();
> +      Succ = Block ? Block : createBlock();
>
> Why this line change?  The "loop back" block is very important for analyzer
> diagnostics.  Is this because 'addAutomaticObjDtors' might create a block?
>
> Yes.

>     }
>
>     // Finish up the increment (or empty) block if it hasn't been already.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100928/cee96c3f/attachment.html>