Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

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

3.3.0
closed
Rick McGuire
Bug fix (24)
5
2012-08-14
2007-11-17
Ruurd Idenburg
No

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.

Discussion

  • Mark Miesfeld
    Mark Miesfeld
    2007-11-18

    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
    Ruurd Idenburg
    2007-11-18

    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
    Mark Miesfeld
    2007-11-25

    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
    Ruurd Idenburg
    2007-11-25

    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
    Rick McGuire
    2007-11-25

    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
    Mark Miesfeld
    2007-11-25

    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'.

     


Anonymous


Cancel   Add attachments