#60 sream_lines expects 'n' or 'N', not OREF_NORMAL

[Ruurd Idenburg]

LINES in BuiltinFunctions.cpp now uses OREF_NORMAL(= 'NORMAL') to pass the Normal option on to stream_lines in StreamLibrary.cpp; but StreamLibrary.cpp still expects just 1 character.

Failing symtioms were:

lines(),lines(,'Count'), lines('No') would fial with

syntax 40, Incorrect call to method.

Patch file for stream_lines in StreamLibrary.cpp attached.

[Ruurd Idenburg]

patch fro bug in stream_lines

[Mark Miesfeld]

Logged In: YES
user_id=191588
Originator: NO

Ruurd,

First, thanks for finding this. I have a couple of comments.

This is a bug and, in my opinion, it is best to track these things by opening up a bug rather than a patch. You can attach your patch to the bug rather than create a "Patch" tracker item.

[Ruurd Idenburg]

Logged In: YES
user_id=1847232
Originator: YES

Mark,

You're right, I forgot one can attach a patch to a bug report as well, will do next time.

[Mark Miesfeld]

Logged In: YES
user_id=191588
Originator: NO

Hi Ruurd,

I've got two points. Hoping Rick will chime in also.

1.) For your patch you have:

if (strQuickFlag != 0)
if (strnicmp(strQuickFlag,"n",1) == 0)
quickflag = 1;
else
if (strnicmp(strQuickFlag,"c",1) != 0 && *strQuickFlag != '\0' )
send_exception(Error_Incorrect_method);

I think the following is a little cleaner and wonder if you have any objection to changing the patch to this:

if (strQuickFlag != 0)
{
char ch = toupper(*strQuickFlag);
if (ch == 'N')
quickflag = 1;
else if (ch != 'C' && ch != '\0')
send_exception(Error_Incorrect_method);
}

2.) Need Rick's opinion on this. In my opinion the existing code is incorrect here:

&& *strQuickFlag != '\0'

Using the empty string is not the same as ommitting an option. So I think the patch should be:

if (strQuickFlag != 0)
{
char ch = toupper(*strQuickFlag);
if (ch == 'N')
quickflag = 1;
else if (ch != 'C')
send_exception(Error_Incorrect_method);
}

(Actually, looking at the difference in the current BuiltinFunctions.cpp and the 3.2.0 BuiltinFunctions.cpp, I think Rick already agrees that the empty string is not valid.)

[Ruurd Idenburg]

Logged In: YES
user_id=1847232
Originator: YES

No objection at all, I too think it's clearer that way. My habit when changing C/C++ code is to change as little as possible. I can read and follow the code Ok, but when it comes to writing I always have to look things up.

About the '\0'; There are more places where I've seen that to test if an argument has been omitted, Rick should probably answer that, as you suggested.

Ruurd

[Rick McGuire]

Logged In: YES
user_id=1125291
Originator: NO

I'm getting errors downloading the patch this morning, so I'm not able to look at the original. But yes, accepting a null string is absolutely not correct. If you see other places where this is done, you might want to mention it....those are likely bugs too.

[Mark Miesfeld]

Logged In: YES
user_id=191588
Originator: NO

Committed revision 1302.

Cleaned up the option check in stream_lines to look for 'N' and 'C'.