[cfe-commits] [Patch] Change the uninitialized field check to use an EvaluatedExprVisitor which will catch more use of uninitialized use.

[Richard Smith]

On Fri, Jun 1, 2012 at 3:09 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Wed, May 30, 2012 at 5:37 PM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> On Fri, May 18, 2012 at 3:35 PM, Richard Trieu <rtrieu at google.com> wrote:
>>
>>> Update the uninitialized field checker to catch more cases of
>>> uninitialized use.  This will catch fields passed by value to function
>>> calls and properly parse conditional operators.  Instead of a recursive
>>> function, an EvaluatedExprVisitor will be used.
>>>
>>
>> What's the compile-time impact of this? Maybe synthesize a test case with
>> a crazy number of fields (all initialized) with long expressions in their
>> initializers, and compare before and after? (There are some tests in INPUTS
>> that you can model this after...)
>>
>
> Synthesized some test cases with 4096 classes, each class having a single
> field initializer with 2048 sub expressions.  My testing showed a 5-10%
> slow-down for the new EvaluatedExprVisitor compared to the current
> recursive function check.

Is that just for the check itself, or an entire -fsyntax-only compile?
If it's the former, that sounds completely reasonable, given that this
is able to catch many more bugs. If it's the latter, it would be worth
at least wrapping the CheckInitExprContainsUninitializedFields loop
with a test for diag::warn_field_is_uninit being enabled.

You have a couple of "T* p"s in the patch which should be "T *p".
Also, HandleValue should recurse into the RHS of a comma operator and
the LHS of a .* or ->*, as well as BinaryConditionalOperator
(basically, this is checking the set of potential results of the
expression, as defined in CWG issue 712).

Other than that, LGTM.