On Wed, 2007-08-08 at 19:01 +0200, grass-dev@grass.itc.it wrote:
code I item #455, was opened at 2007-08-08 12:01
Status: Open
Priority: 3
Submitted By: William Kyngesburye (kyngchaos)
Assigned to: Nobody (None)
Summary: r.proj segfaults after Jul 14 changes
Issue type: module bug
Issue status: None
GRASS version: CVS HEAD
GRASS component: raster
Operating system: MacOS X
Operating system version: 10.4
GRASS CVS checkout date, if applies (YYMMDD):
Fixed in CVS. That was improper usage of G_done_msg() as noticed.
Actually, it's more than just G_done_msg - there are a few G_message(NULL) in there, and the r.proj source is really in r.proj.seg, the r.proj source folder is deprecated.
Not to mention the other modules I found null messages in, as I mentioned in a followup to the bug report.
If (NULL) is indeed improper use of the message functions, I could take care of them - I have my own CVS copy of r.proj.seg updated already, and I found those others already.
On Aug 8, 2007, at 12:37 PM, Brad Douglas wrote:
On Wed, 2007-08-08 at 19:01 +0200, grass-dev@grass.itc.it wrote:
code I item #455, was opened at 2007-08-08 12:01
Status: Open
Priority: 3
Submitted By: William Kyngesburye (kyngchaos)
Assigned to: Nobody (None)
Summary: r.proj segfaults after Jul 14 changes
Issue type: module bug
Issue status: None
GRASS version: CVS HEAD
GRASS component: raster
Operating system: MacOS X
Operating system version: 10.4
GRASS CVS checkout date, if applies (YYMMDD):
Fixed in CVS. That was improper usage of G_done_msg() as noticed.
Actually, it's more than just G_done_msg - there are a few G_message(NULL) in there, and the r.proj source is really in r.proj.seg, the r.proj source folder is deprecated.
Not to mention the other modules I found null messages in, as I mentioned in a followup to the bug report.
If (NULL) is indeed improper use of the message functions, I could take care
Well apart from passing NULL instead of an empty string being obviously wrong, a message that actually contains no information would appear to be improper use by definition, no? What's the point of it?
Just seems like an obvious remark to make anyway...
> Actually, it's more than just G_done_msg - there are a few G_message(NULL) in
> there, and the r.proj source is really in r.proj.seg, the r.proj source
> folder is deprecated.
>
> Not to mention the other modules I found null messages in, as I mentioned in
> a followup to the bug report.
>
> If (NULL) is indeed improper use of the message functions, I could take care
Well apart from passing NULL instead of an empty string being obviously
wrong, a message that actually contains no information would appear to be
improper use by definition, no? What's the point of it?
Just seems like an obvious remark to make anyway...
The G_message(NULL) calls appear between the code which prints the
input and output regions. IOW, they're there to add blank lines.
Personally, I think that section should use G_verbose_message().
On Wed, 2007-08-08 at 12:54 -0500, William Kyngesburye wrote:
Actually, it's more than just G_done_msg - there are a few G_message
(NULL) in there, and the r.proj source is really in r.proj.seg, the
r.proj source folder is deprecated.
Ugh.
Not to mention the other modules I found null messages in, as I
mentioned in a followup to the bug report.
If (NULL) is indeed improper use of the message functions, I could
take care of them - I have my own CVS copy of r.proj.seg updated
already, and I found those others already.
That is blatantly wrong. Does anyone actually test code they write?
Please commit your changes. I'll update the documentation.
Not to mention the other modules I found null messages in, as I
mentioned in a followup to the bug report.
If (NULL) is indeed improper use of the message functions, I could
take care of them - I have my own CVS copy of r.proj.seg updated
already, and I found those others already.
That is blatantly wrong. Does anyone actually test code they write?
Please commit your changes. I'll update the documentation.
That's interesting, because when I looked at the r.proj main.c history, the "Message/Debug cleaning" revision explicitly changed a couple G_message("") to G_message(NULL) (among other changes).
To be fair, it may be that it worked for them (it only seems to cause trouble on OSX). They may or may not *know* if it's wrong (I didn't, tho I understood what it was meant to do).
Anyways, I've changed all the NULL messages I found in CVS now.
There is a theory which states that if ever anyone discovers exactly what the universe is for and why it is here, it will instantly disappear and be replaced by something even more bizarrely inexplicable. There is another theory which states that this has already happened.
-Hitchhiker's Guide to the Galaxy 2nd season intro
On Thu, 2007-08-09 at 09:27 -0500, William Kyngesburye wrote:
On Aug 9, 2007, at 1:57 AM, Brad Douglas wrote:
>> Not to mention the other modules I found null messages in, as I
>> mentioned in a followup to the bug report.
>>
>> If (NULL) is indeed improper use of the message functions, I could
>> take care of them - I have my own CVS copy of r.proj.seg updated
>> already, and I found those others already.
>
> That is blatantly wrong. Does anyone actually test code they write?
> Please commit your changes. I'll update the documentation.
>
That's interesting, because when I looked at the r.proj main.c
history, the "Message/Debug cleaning" revision explicitly changed a
couple G_message("") to G_message(NULL) (among other changes).
This is cause for some concern. If unsure, glance at the function code.
It would have been more than obvious.
To be fair, it may be that it worked for them (it only seems to cause
trouble on OSX). They may or may not *know* if it's wrong (I didn't,
tho I understood what it was meant to do).
Not all NULLs are treated equally by all systems. Turning -Wall on
should have revealed it, too. It may have been an honest mistake, but
it was a careless mistake.
Anyways, I've changed all the NULL messages I found in CVS now.
Thank you. I updated the documentation this morning to explicitly warn
against it.
--
Brad Douglas <rez touchofmadness com> KB8UYR/6
Address: 37.493,-121.924 / WGS84 National Map Corps #TNMC-3785
Well apart from passing NULL instead of an empty string being obviously
wrong, a message that actually contains no information would appear to be
improper use by definition, no? What's the point of it?
It's valid for G_done_msg(), it outputs like:
$MODULE complete. $*
with "", it just says "$MOD complete. " So passing a message is optional extra.
However, G_message(""); for a blank line is ugly ugly, I'd suggest either
adding a new lib fn (lame solution) or removing whitespace stripping action
from G_message(), and allow '\n' back in the string. The problem with this is
that self-formatting gets abused, and perhaps confuses the line-wrap code?
(this is just off the top of my head, I can't check what the code actually does
right now)
Hamish
____________________________________________________________________________________
Take the Internet to Go: Yahoo!Go puts the Internet in your pocket: mail, news, photos & more. http://mobile.yahoo.com/go?refer=1GNXIC
Well apart from passing NULL instead of an empty string being obviously
wrong, a message that actually contains no information would appear to be
improper use by definition, no? What's the point of it?
It's valid for G_done_msg(), it outputs like:
$MODULE complete. $*
with "", it just says "$MOD complete. " So passing a message is optional extra.
If it is intended to be optional for G_done_msg, then this needs to be fixed. It's the vsprintf(buffer,msg,ap); part in there that is causing trouble on OSX. If NULL is valid, then a check for NULL should be done before vsprintf is called.
"This is a question about the past, is it? ... How can I tell that the past isn't a fiction designed to account for the discrepancy between my immediate physical sensations and my state of mind?"
However, G_message(""); for a blank line is ugly ugly, I'd suggest either
adding a new lib fn (lame solution) or removing whitespace stripping action
from G_message(), and allow '\n' back in the string. The problem with this is
Well yes - if it does that then that explains why people are passing an empty message. It's acting more like G_print_this_info_to_stderr() than a real G_message(). IMHO a message should be a distinct piece of information, that may or may not require multiple lines to get across. The calling modules should be able to add line breaks if they see fit. I also think G_message(), G_warning() etc. should only break lines if output is going to a terminal. When the messages are re-processed and displayed through the GUI the extraneous line breaks can look quite ugly in certain circumstances (e.g. pop-up dialog boxes containing warnings).
Allowing newlines in messages passed to G_message() but then stripping them out strikes me as really messy, and just lazy - why not fix all the calls to G_message() instead to conform to the conventions?
that self-formatting gets abused, and perhaps confuses the line-wrap code?
Hmm, what do you mean by abused? I feel that the programmer should be able to put newlines into a message if it is necessary to get the meaning across, and the function that prints/parses the output should respect the newlines.
On Fri, 2007-08-10 at 08:52 -0500, William Kyngesburye wrote:
On Aug 9, 2007, at 5:46 PM, Hamish wrote:
> Paul Kelly wrote:
>> Well apart from passing NULL instead of an empty string being
>> obviously
>> wrong, a message that actually contains no information would
>> appear to be
>> improper use by definition, no? What's the point of it?
>
> It's valid for G_done_msg(), it outputs like:
> $MODULE complete. $*
>
> with "", it just says "$MOD complete. " So passing a message is
> optional extra.
If it is intended to be optional for G_done_msg, then this needs to
be fixed. It's the vsprintf(buffer,msg,ap); part in there that is
causing trouble on OSX. If NULL is valid, then a check for NULL
should be done before vsprintf is called.
I'm opposed to adding largely unnecessary code. The console is NOT
designed for well laid out display. It is there for informational
purposes only. IMO, if you want pretty messages, use a GUI.
--
Brad Douglas <rez touchofmadness com> KB8UYR/6
Address: 37.493,-121.924 / WGS84 National Map Corps #TNMC-3785
On Fri, 2007-08-10 at 08:52 -0500, William Kyngesburye wrote:
If it is intended to be optional for G_done_msg, then this needs to
be fixed. It's the vsprintf(buffer,msg,ap); part in there that is
causing trouble on OSX. If NULL is valid, then a check for NULL
should be done before vsprintf is called.
I'm opposed to adding largely unnecessary code. The console is NOT
designed for well laid out display. It is there for informational
purposes only. IMO, if you want pretty messages, use a GUI.
It's not a matter of changing the way it prints the message. The problem is that passing NULL to vsprintf() on OSX causes a bus error. If NULL is meant to be valid for G_done_msg(), then it needs to trap that so it doesn't pass it to vsprintf().
On Fri, 2007-08-10 at 12:07 -0500, William Kyngesburye wrote:
On Aug 10, 2007, at 11:32 AM, Brad Douglas wrote:
> On Fri, 2007-08-10 at 08:52 -0500, William Kyngesburye wrote:
>>
>> If it is intended to be optional for G_done_msg, then this needs to
>> be fixed. It's the vsprintf(buffer,msg,ap); part in there that is
>> causing trouble on OSX. If NULL is valid, then a check for NULL
>> should be done before vsprintf is called.
>
> I'm opposed to adding largely unnecessary code. The console is NOT
> designed for well laid out display. It is there for informational
> purposes only. IMO, if you want pretty messages, use a GUI.
>
It's not a matter of changing the way it prints the message. The
problem is that passing NULL to vsprintf() on OSX causes a bus
error. If NULL is meant to be valid for G_done_msg(), then it needs
to trap that so it doesn't pass it to vsprintf().
How many times to I have to state that passing NULL to *printf() va*()
is absolutely, positively **WRONG**? And we should NOT be modifying the
API to accommodate one person's stylistic choices.
Is anyone listening?
G_msg_done() is not living up to its intended use. It should probably
be deprecated.
--
Brad Douglas <rez touchofmadness com> KB8UYR/6
Address: 37.493,-121.924 / WGS84 National Map Corps #TNMC-3785
Sorry to cause a ruckus... Somewhere along the way it was suggested (maybe I misinterpreted that) that calling G_done_msg() with NULL was acceptable, though I completely understand that passing NULL to vsprintf is wrong (it just happens to work on some platforms).
On Aug 10, 2007, at 12:20 PM, Brad Douglas wrote:
How many times to I have to state that passing NULL to *printf() va*()
is absolutely, positively **WRONG**? And we should NOT be modifying the
API to accommodate one person's stylistic choices.
"This is a question about the past, is it? ... How can I tell that the past isn't a fiction designed to account for the discrepancy between my immediate physical sensations and my state of mind?"
> Paul Kelly wrote:
>> Well apart from passing NULL instead of an empty string being
>> obviously
>> wrong, a message that actually contains no information would
>> appear to be
>> improper use by definition, no? What's the point of it?
Hamish:
> It's valid for G_done_msg(), it outputs like:
> $MODULE complete. $*
>
> with "", it just says "$MOD complete. " So passing a message is
> optional extra.
William:
If it is intended to be optional for G_done_msg, then this needs to
be fixed. It's the vsprintf(buffer,msg,ap); part in there that is
causing trouble on OSX. If NULL is valid, then a check for NULL
should be done before vsprintf is called.
Sorry, what I meant was it is valid for G_done_msg() to have an empty string as
its argument, not that the empty string could be sent as NULL.
Hamish
____________________________________________________________________________________
Moody friends. Drama queens. Your life? Nope! - their life, your story. Play Sims Stories at Yahoo! Games. http://sims.yahoo.com/
I'm opposed to adding largely unnecessary code. The console is NOT
designed for well laid out display. It is there for informational
purposes only. IMO, if you want pretty messages, use a GUI.
A main reason for using G_done_msg() is that when run from a GUI it is not
clear when the module has finished. Preferably the module's GUI output would
print:
[green check mark] "$MODULE complete."
when the process has stopped but in the past (tcl gui) it was unclear how to do
that.
If the module is run from the command line the message is mostly redundant.
Right now from the GUI you get output like:
Loading raster map <> ...
Processing ...
Writing map ...
n features cleaned
But there really isn't any indication that the module is done.
so really G_done_mgs() use is a sypmtom of a GUI shortcoming which is impinging
onto other things.
Hamish
____________________________________________________________________________________
Park yourself in front of a world of choices in alternative vehicles. Visit the Yahoo! Auto Green Center. http://autos.yahoo.com/green_center/
A main reason for using G_done_msg() is that when run from a GUI it is not
clear when the module has finished. Preferably the module's GUI output would
print:
[green check mark] "$MODULE complete."
when the process has stopped but in the past (tcl gui) it was unclear how to do
that.
If the module is run from the command line the message is mostly redundant.
Right now from the GUI you get output like:
Loading raster map <> ...
Processing ...
Writing map ...
n features cleaned
But there really isn't any indication that the module is done.
so really G_done_mgs() use is a sypmtom of a GUI shortcoming which is impinging
onto other things.
ISTR if you look at the "title bar" for the currently running module in the gronsole screen there is a running man icon when the module is running, and it disappears when the module completes. Could def. be more obvious though. And if there is a lot of output it scrolls off the top of the screen and you don't see it.
Brad Douglas wrote:
> I'm opposed to adding largely unnecessary code. The console is NOT
> designed for well laid out display. It is there for informational
> purposes only. IMO, if you want pretty messages, use a GUI.
A main reason for using G_done_msg() is that when run from a GUI it is not
clear when the module has finished. Preferably the module's GUI output would
print:
[green check mark] "$MODULE complete."
when the process has stopped but in the past (tcl gui) it was unclear how to do
that.
If the module is run from the command line the message is mostly redundant.
Right now from the GUI you get output like:
Loading raster map <> ...
Processing ...
Writing map ...
n features cleaned
But there really isn't any indication that the module is done.
so really G_done_mgs() use is a sypmtom of a GUI shortcoming which is impinging
onto other things.
What I meant by "G_done_msg () is not living up to it's intended use" is
that it should be re-implemented as a void function. Allowing people to
add text to the string is causing too much confusion.
It would be much easier to re-declare as 'G_done_msg (void)' and append
it to modules that don't already use it to indicate completion. IMO, it
should either be used everywhere or gotten rid of.