Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
Hi Graham, Carl, On Tue 23 Aug 2011 19:34:37 BST, Carl Sorensen wrote: On 8/23/11 12:21 PM, ianhuli...@gmail.com ianhuli...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
Ian Hulin ianhuli...@gmail.com writes:
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.
I disagree. Structured exits decrease the level of nesting and
contortion and make code more straightforward to follow.
if (!precondition)
{
return 0;
}
// Do the real work, 200+ lines of code, 5 levels of nesting
return zappa (whatever);
is much much more cleaner to follow than
zappadap result;
if (precondition)
{
// Do the real work, 200+ lines of code, 5 levels of nesting
result = zappa (whatever);
}
else
{
result = 0;
}
return result;
Get the trivial cases out of the way, completely, first. That way, you
don't need to keep yellow tabs on braces to track even the trivial code.
Write and read code like a man! Juggling a dozen open ends at once is
the special skillset of childraisers, not programmers. And if you do
both at once, you'll be glad you don't need to track large contexts even
for the trivial tasks, when you have a spare minute for programming.
--
David Kastrup
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
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, ianhuli...@gmail.com ianhuli...@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, reinh...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
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, ianhuli...@gmail.com ianhuli...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
Dan Eble d...@faithful.be writes: 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. Modern compilers pay very little attention to how you arrange the source code of equivalent constructs. You can also write functions that are equally difficult to read with both. No. Humans begin reading at the beginning and go forward from there. There is a difference between having one case dealt with completely, and having to keep it in mind while reading something else. Basically you are saying juggling with one ball can be equally difficult to juggling with two and conclude from that dubious premise that juggling any number of balls is equally hard. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
On Tue, Aug 23, 2011 at 3:21 PM, ianhuli...@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 - han...@xs4all.nl - http://www.xs4all.nl/~hanwen ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
On 2011-08-24, at 09:25 , David Kastrup wrote: Modern compilers pay very little attention to how you arrange the source code of equivalent constructs. My experience trying to finagle optimized code out of gcc was more than a year ago, and the compiler was probably a bit older than that (not much though--I remember it was upgraded shortly before I was given the assignment). I see in the documentation for gcc 4.7.0 that by default it tries to guess the probability of branching, so it seems you are right about 'modern' compilers (one of them anyway). What a difference a few years makes. Thanks for the lesson, -- Dan ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
Reviewers: ,
Message:
Please review to get rid of some uninitialized variables.
Description:
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.
Please review this at http://codereview.appspot.com/4940047/
Affected files:
M lily/source-file.cc
Index: lily/source-file.cc
diff --git a/lily/source-file.cc b/lily/source-file.cc
index
b42fb7a5b37a508f69ce8d31925ae9478e7972c4..041c046d2bfc2fa4761df56005f2e76692f962f7
100644
--- a/lily/source-file.cc
+++ b/lily/source-file.cc
@@ -261,7 +261,11 @@ Source_file::get_counts (char const *pos_str0,
int *column,
int *byte_offset) const
{
+ // Initialize arguments to defaults, needed if pos_str0 is not in source
*line_number = 0;
+ *line_char = 0;
+ *column = 0;
+ *byte_offset = 0;
if (!contains (pos_str0))
return;
@@ -276,10 +280,6 @@ Source_file::get_counts (char const *pos_str0,
string line_begin (line_start, left);
char const *line_chars = line_begin.c_str ();
- *line_char = 0;
- *column = 0;
- *byte_offset = 0;
-
while (left 0)
{
size_t thislen = utf8_char_len (*line_chars);
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
LGTM. Thanks! Carl http://codereview.appspot.com/4940047/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
On Tue, Aug 23, 2011 at 11:14:06AM +, reinhold.kainho...@gmail.com wrote: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) LGTM Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
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 http://codereview.appspot.com/4940047/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
On Tue, Aug 23, 2011 at 06:21:00PM +, ianhuli...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)
On 8/23/11 12:21 PM, ianhuli...@gmail.com ianhuli...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
