Initialized pair_id variable to 0....

Initialized pair_id variable to 0.

This variable was uninitialized and the compiler gave a warning (which
    got converted into an error because we compile using -Werror) when
GetIDAndCountOfFormElement() and its children are inlined. This is
because there is a path in that function that returns true but leaves
the pair_id variable uninitialized (if s.Step() is 0).

Note that I am not a Chromium committer so you will have to commit
this.

BUG=70144
TEST=Submitting to trybots now.

[asharif1, 2011-01-19 19:23:07]

Please review this.

[Avi (use Gerrit), 2011-01-19 19:42:02]

NAK, this feels wrong.

If GetIDAndCountOfFormElement() can return true yet not actually return a valid
value (which appears to be the case), you should fix it rather than its callers.
Is it correct in the case where s.Step() is 0 to return false?

[dhollowa, 2011-01-19 21:08:03]

According to the .h:

  Sets *count to 0 if there is no such row in the table.

So setting pair_id to 0 and returning true seems like the correct path. 
According to docs for |sqlite3_last_insert_rowid| a pair_id of 0 signals an
invalid row, so that won't clash with a valid row id.

On 2011/01/19 19:42:02, Avi wrote:
> NAK, this feels wrong.
> 
> If GetIDAndCountOfFormElement() can return true yet not actually return a
valid
> value (which appears to be the case), you should fix it rather than its
callers.
> Is it correct in the case where s.Step() is 0 to return false?

[dhollowa, 2011-01-19 21:09:57]

Also, I'm curious.  Which compiler is complaining?

On 2011/01/19 21:08:03, dhollowa wrote:
> According to the .h:
> 
>   Sets *count to 0 if there is no such row in the table.
> 
> So setting pair_id to 0 and returning true seems like the correct path. 
> According to docs for |sqlite3_last_insert_rowid| a pair_id of 0 signals an
> invalid row, so that won't clash with a valid row id.
> 
> On 2011/01/19 19:42:02, Avi wrote:
> > NAK, this feels wrong.
> > 
> > If GetIDAndCountOfFormElement() can return true yet not actually return a
> valid
> > value (which appears to be the case), you should fix it rather than its
> callers.
> > Is it correct in the case where s.Step() is 0 to return false?

[asharif1, 2011-01-19 21:33:40]

This is Crosstool (Google compiler) which does more aggressive inlining than
vanilla gcc.

This "bug" was exposed because it inlined that function otherwise just by
looking at one function at a time the compiler cannot know whether there are
uninitialized variables.

So should I set it to 0 and return true in the uninitialized path?

On Wed, Jan 19, 2011 at 1:09 PM, <dhollowa@chromium.org> wrote:

> Also, I'm curious.  Which compiler is complaining?
>
>
> On 2011/01/19 21:08:03, dhollowa wrote:
>
>> According to the .h:
>>
>
>    Sets *count to 0 if there is no such row in the table.
>>
>
>  So setting pair_id to 0 and returning true seems like the correct path.
>> According to docs for |sqlite3_last_insert_rowid| a pair_id of 0 signals
>> an
>> invalid row, so that won't clash with a valid row id.
>>
>
>  On 2011/01/19 19:42:02, Avi wrote:
>> > NAK, this feels wrong.
>> >
>> > If GetIDAndCountOfFormElement() can return true yet not actually return
>> a
>> valid
>> > value (which appears to be the case), you should fix it rather than its
>> callers.
>> > Is it correct in the case where s.Step() is 0 to return false?
>>
>
>
>
> http://codereview.chromium.org/6293011/
>

[dhollowa, 2011-01-19 21:47:08]

Yes, I would simply add:

   *pair_id = 0;

just before |*count = 0;| on line 1204.

And update the header file comment for |GetIDAndCountOfFormElement| to be clear
about what happens to the |pair_id| parameter.

On 2011/01/19 21:33:40, asharif1 wrote:
> This is Crosstool (Google compiler) which does more aggressive inlining than
> vanilla gcc.
> 
> This "bug" was exposed because it inlined that function otherwise just by
> looking at one function at a time the compiler cannot know whether there are
> uninitialized variables.
> 
> So should I set it to 0 and return true in the uninitialized path?
> 
> On Wed, Jan 19, 2011 at 1:09 PM, <mailto:dhollowa@chromium.org> wrote:
> 
> > Also, I'm curious.  Which compiler is complaining?
> >
> >
> > On 2011/01/19 21:08:03, dhollowa wrote:
> >
> >> According to the .h:
> >>
> >
> >    Sets *count to 0 if there is no such row in the table.
> >>
> >
> >  So setting pair_id to 0 and returning true seems like the correct path.
> >> According to docs for |sqlite3_last_insert_rowid| a pair_id of 0 signals
> >> an
> >> invalid row, so that won't clash with a valid row id.
> >>
> >
> >  On 2011/01/19 19:42:02, Avi wrote:
> >> > NAK, this feels wrong.
> >> >
> >> > If GetIDAndCountOfFormElement() can return true yet not actually return
> >> a
> >> valid
> >> > value (which appears to be the case), you should fix it rather than its
> >> callers.
> >> > Is it correct in the case where s.Step() is 0 to return false?
> >>
> >
> >
> >
> > http://codereview.chromium.org/6293011/
> >

[asharif1, 2011-01-19 22:11:29]

On 2011/01/19 21:47:08, dhollowa wrote:
> Yes, I would simply add:
> 
>    *pair_id = 0;
> 
> just before |*count = 0;| on line 1204.
> 
> And update the header file comment for |GetIDAndCountOfFormElement| to be
clear
> about what happens to the |pair_id| parameter.
> 
> On 2011/01/19 21:33:40, asharif1 wrote:
> > This is Crosstool (Google compiler) which does more aggressive inlining than
> > vanilla gcc.
> > 
> > This "bug" was exposed because it inlined that function otherwise just by
> > looking at one function at a time the compiler cannot know whether there are
> > uninitialized variables.
> > 
> > So should I set it to 0 and return true in the uninitialized path?
> > 
> > On Wed, Jan 19, 2011 at 1:09 PM, <mailto:dhollowa@chromium.org> wrote:
> > 
> > > Also, I'm curious.  Which compiler is complaining?
> > >
> > >
> > > On 2011/01/19 21:08:03, dhollowa wrote:
> > >
> > >> According to the .h:
> > >>
> > >
> > >    Sets *count to 0 if there is no such row in the table.
> > >>
> > >
> > >  So setting pair_id to 0 and returning true seems like the correct path.
> > >> According to docs for |sqlite3_last_insert_rowid| a pair_id of 0 signals
> > >> an
> > >> invalid row, so that won't clash with a valid row id.
> > >>
> > >
> > >  On 2011/01/19 19:42:02, Avi wrote:
> > >> > NAK, this feels wrong.
> > >> >
> > >> > If GetIDAndCountOfFormElement() can return true yet not actually return
> > >> a
> > >> valid
> > >> > value (which appears to be the case), you should fix it rather than its
> > >> callers.
> > >> > Is it correct in the case where s.Step() is 0 to return false?
> > >>
> > >
> > >
> > >
> > > http://codereview.chromium.org/6293011/
> > >

Take a look at the CL again. LGTM and submit on my behalf if you think this is
ready.

[Avi (use Gerrit), 2011-01-19 22:14:17]

Much better. LGTM.

[Avi (use Gerrit), 2011-01-19 22:34:58]

Committed as r71860.