Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0)

Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0)

Most code that called get_counts simply is like:
  int line, chr, col, offset = 0;
  source_file_->get_counts (end_, &line, &chr, &col, &offset);

Now, unfortunately get_counts returns early sometimes (if we don't have a position), so 
only line_number would be initialized to 0, all other variables would stay uninitialized.
And most code simply passed them on to other guile functions to handle.

This patch moved the initialization of all arguments to the very beginning of get_counts
and thus never returns uninizialized variables.

This shuts up several valgrind warnings in our regtests.

[Reinhold, 2011-08-23 11:14:06]

Please review to get rid of some uninitialized variables.

[Carl, 2011-08-23 12:03:14]

LGTM.

Thanks!

Carl

[Graham Percival, 2011-08-23 17:27:47]

On Tue, Aug 23, 2011 at 11:14:06AM +0000, reinhold.kainhofer@gmail.com wrote:
> Fix uninitialized variables when Source_file::get_counts returns early
> due to !contains (pos_str0)

LGTM

Cheers,
- Graham

[Ian Hulin (gmail), 2011-08-23 18:21:00]

LGTM

Maybe we should have some GOP rules for C++ about this?
"Only have multiple exit points from routines if you absolutely have to.
Make sure any output parameters are declared and initialized at the top of a
routine so that however a routine exits, they are left in a defined state"
Ian

[Graham Percival, 2011-08-23 18:29:05]

On Tue, Aug 23, 2011 at 06:21:00PM +0000, ianhulin44@gmail.com wrote:
> Maybe we should have some GOP rules for C++ about this?
> "Only have multiple exit points from routines if you absolutely have to.
> Make sure any output parameters are declared and initialized at the top
> of a routine so that however a routine exits, they are left in a defined
> state"

I don't think we need to wait that long (i.e. at least a month, if
not 2 or 3 months to get through GOP).  Regardless of the exit
points, any member variables or output parameters should be left
in a defined state.  IMO that's just basic sanity.  :)

Cheers,
- Graham

[c_sorensen, 2011-08-23 18:34:49]

On 8/23/11 12:21 PM, "ianhulin44@gmail.com" <ianhulin44@gmail.com> wrote:

> LGTM
> 
> Maybe we should have some GOP rules for C++ about this?
> "Only have multiple exit points from routines if you absolutely have to.

Multiple exit points is a standard idiom of the LilyPond code.  Basically,
the idiom is "If this routine doesn't apply for some reason, leave now",
instead of "If this routine doesn't apply, skip to the end" or "If this
routine applies, do the body".

As imbedded as this idiom is in our code, I don't think it would be wise to
change it.

> Make sure any output parameters are declared and initialized at the top
> of a routine so that however a routine exits, they are left in a defined
> state"

IIRC, we used to have a statement that said to declare variables as close as
possible to where they are used.  I generally agree with that.

However, I think a simple statement that says "variables should be
initialized when they are declared" would be clearly welcome.  And fixes to
the code to ensure this would also be welcome.

Thanks,

Carl

[Ian Hulin (gmail), 2011-08-24 08:49:31]

Hi  Graham, Carl,

On Tue 23 Aug 2011 19:34:37 BST, Carl Sorensen wrote:
> On 8/23/11 12:21 PM, "ianhulin44@gmail.com" <ianhulin44@gmail.com> wrote:
> 
>> LGTM
>>
>> Maybe we should have some GOP rules for C++ about this?
>> "Only have multiple exit points from routines if you absolutely have to.
> 
> Multiple exit points is a standard idiom of the LilyPond code.  Basically,
> the idiom is "If this routine doesn't apply for some reason, leave now",
> instead of "If this routine doesn't apply, skip to the end" or "If this
> routine applies, do the body".
> 
> As imbedded as this idiom is in our code, I don't think it would be wise to
> change it.
> 
... a sort of "legal precedent" argument.  Just because it's precedent, 
doesn't make it right.
"leave now" is an optimisation: it means you're assuming everything is 
safe and you know for certain that the code you have executed up to the 
point of exit *including any routines it has called* has done no harm.
Branching to a final single exit point allows you to clean up any 
side-effects/damage/half-completed stuff your code changed thus far.
Current practice didn't work in this case.  

I'm not asking for a grand re-write on this, but for single-exit to be 
the preferred style for new code and patches where this would not 
provoke changes on a GCR (Grand Code Re-write) scale.

>> Make sure any output parameters are declared and initialized at the top
>> of a routine so that however a routine exits, they are left in a defined
>> state"
> 
> IIRC, we used to have a statement that said to declare variables as close as
> possible to where they are used.  I generally agree with that.
> 
> However, I think a simple statement that says "variables should be
> initialized when they are declared" would be clearly welcome.  And fixes to
> the code to ensure this would also be welcome.
> 
OK, maybe take the point above as a stylistic preference.  

Declaring variables nearest to where they are used + insisting they are 
initialised may work, providing we spell it out the if they are 
declared as parameters to the routine, that's the first place they're 
used, so if they're output parameters, they need to be initialized 
either at the top of the routine, or in the parameter declaration.

Tightening up this statement may do the trick.  We need to feed back 
things we pick up on like this into the development/reviewing process.

Cheers,
Ian

[reinhold_kainhofer.com, 2011-08-24 09:10:37]

Am Wednesday, 24. August 2011, 10:49:30 schrieben Sie:
> Hi  Graham, Carl,
> 
> On Tue 23 Aug 2011 19:34:37 BST, Carl Sorensen wrote:
> > On 8/23/11 12:21 PM, "ianhulin44@gmail.com" <ianhulin44@gmail.com> wrote:
> >> Maybe we should have some GOP rules for C++ about this?
> >> "Only have multiple exit points from routines if you absolutely have to.

There is really not much difference in

if (some_condition)
  return;

and

if (!some_condition)
  {
    ....
  }

Except that if you have many conditions the indentation in the second example 
will be terrible and make the whole thing hard to read.

You can mess up equally with both.

> Declaring variables nearest to where they are used + insisting they are
> initialised may work, providing we spell it out the if they are
> declared as parameters to the routine, that's the first place they're
> used, so if they're output parameters, they need to be initialized
> either at the top of the routine, or in the parameter declaration.

Please note that in this case we are talking about a function that is expected 
to modify its arguments, this is NOT about variable declarations. These 
variables, passed-by-reference, should be initialized as soon as possible.

For variable declarations, of course I'm all for moving the declaration 
immediately before the first use.

Cheers,
Reinhold

-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

[dan_faithful.be, 2011-08-24 13:01:59]

On 2011-08-24, at 05:10 , Reinhold Kainhofer wrote:

> Am Wednesday, 24. August 2011, 10:49:30 schrieben Sie:
>> Hi  Graham, Carl,
>> 
>> On Tue 23 Aug 2011 19:34:37 BST, Carl Sorensen wrote:
>>> On 8/23/11 12:21 PM, "ianhulin44@gmail.com" <ianhulin44@gmail.com> wrote:
>>>> Maybe we should have some GOP rules for C++ about this?
>>>> "Only have multiple exit points from routines if you absolutely have to.
> 
> There is really not much difference in
> 
> if (some_condition)
>  return;
> 
> and
> 
> if (!some_condition)
>  {
>    ....
>  }
> 
> Except that if you have many conditions the indentation in the second example 
> will be terrible and make the whole thing hard to read.


There can be a run-time performance difference between branching or not
branching.  For the times you actually care, if you're not going to use
compiler-specific features to mark conditions as likely or unlikely, you should
test the likely case first so that there is no branching most of the time.

> You can mess up equally with both.

Some of us have. :-)

You can also write functions that are equally difficult to read with both.  The
readability depends more on what is inside your ellipsis above than whether the
unexpected condition is handled before or after it.
-- 
Dan

[hanwenn, 2011-08-24 13:31:23]

On Tue, Aug 23, 2011 at 3:21 PM,  <ianhulin44@gmail.com> wrote:
> LGTM
>
> Maybe we should have some GOP rules for C++ about this?

Can we not? Professionally, I work with an enormous style guide, and
having a lot of style prescribed needlessly complicates code reviews,
because it makes people hammer on unimportant syntactical details.

We could adopt the following GOP rule about writing functions:

 Use your brain when you write and arrange a function.


-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen