bugGNU Octave - Bugs: bug #57879, strncmp(str_a, str_b, N) gives an...

 
 

bug #57879: strncmp(str_a, str_b, N) gives an error for N=0

Submitted by:  Utkarsh Dhandhania <yoodee>
Submitted on:  Fri 21 Feb 2020 11:44:25 PM UTC  
 
Category:  Octave Function Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Confirmed Assigned to:  None
Originator Name:  Utkarsh Dhandhania Open/Closed:  Open
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Thu 12 Mar 2020 10:47:54 PM UTC, comment #34: 

a new patch follow guidelines is comment #32

(file #48590)

NADER NABIL MANSOUR <nadern96>
Thu 12 Mar 2020 10:39:59 PM UTC, comment #33: 

Could those details be added to https://wiki.octave.org/Commit_message_guidelines
by someone who won't mangle it?

Nicholas Jankowski <nrjank>
Thu 12 Mar 2020 09:21:35 PM UTC, comment #32: 

FWIW, the preferred entry format is documented in the GNU standards (https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html). And built-in functions are name-mangled with an "F" prefix. So this might look something like

* strfns.cc (Fstrncmp, Fstrncmpi): Don't throw error when n is 0.
Update docstrings.  Add new BIST tests.
* NEWS: Announce changes to strncmp and strncmpi.

Mike Miller <mtmiller>
Project Administrator
Thu 12 Mar 2020 08:59:37 PM UTC, comment #31: 

The commit message, NEWS file, and docstring changes still need some significant English rewrites before applying. But the rest of this patch looks very good.

It would also be better if you would configure your git and hg commands to use your full name instead of your user name, unless you prefer anonymity, which doesn't seem to be the case here.

Nader, thanks for your work on fixing this bug. If you are interested in contributing to Octave, please do continue to study how Octave defines functions, handles errors, documents functions and arguments, and includes unit tests, this bug only scratches the surface.

Mike Miller <mtmiller>
Project Administrator
Thu 12 Mar 2020 09:31:02 AM UTC, comment #30: 

Edit commit message to be like the message in comment #29

(file #48585)

NADER NABIL MANSOUR <nadern96>
Thu 12 Mar 2020 02:03:03 AM UTC, comment #29: 

this has definitely come along well, appreciate you sticking with it. 

for the commit message:

the first line should be the general description and include the bug number. it shouldn't just be the bug title, it should be a description of what was fixed.  following that, skip a line, and then one line beginning with a * for each file/function changed.  you changed two functions within  strftn.cc and NEWS.  So, I would suggest something like:

strncmp and strncmpi now produce true for N=0 (bug #57879)

* libinterp/corefcn/strfns.cc (strncmp, strncmpi): N=0 now produces a matlab compatible TRUE result, added notes in help about N=0 compatibility change and deliberately retaining N<0 --> false incompatibility
* NEWS: added note to compatibility section

(note that I don't work on compiled functions, so I'm not sure if that function callout is quite right)

to see examples, from within mercurial type:

hg log --limit 10 --verbose

you'll see the last 10 examples.  the 'descriptions' are the commit messages.  You'll see they follow the form I mentioned above.

there are also examples that can almost be copied at:
https://wiki.octave.org/Commit_message_guidelines#Examples

Nicholas Jankowski <nrjank>
Thu 12 Mar 2020 12:45:43 AM UTC, comment #28: 

I submitted a new patch considering the changes in comment #27

(file #48584)

NADER NABIL MANSOUR <nadern96>
Thu 12 Mar 2020 12:19:58 AM UTC, comment #27: 

Yes, there is no reason to modify .hgignore, that part of this patch should be removed.

I don't think we need to have an example in the NEWS file, just the description of the change.

The rest of this looks pretty good. I think we need to do some rewording and reformatting for the text, we can understand the difficulty if English is not your first language.

I would like to see a test that shows a true return value with completely unequal strings, for example

%!assert <*57879> (strncmp ("abc", "def", 0))

Note that you don't need to pass 'true' as the second argument to 'assert' if the first argument is a logical test. For example

assert (1, 1)            ## test two arguments for equality
assert (1 == 1)          ## test a single expressing for truth
assert (1 == 1, true)    ## second argument isn't needed

Mike Miller <mtmiller>
Project Administrator
Wed 11 Mar 2020 07:03:31 PM UTC, comment #26: 

I have submitted a new patch that follows all the guidelines and notes in the comment #19 , comment #22 , comment #23 , comment #12 , comment #25.

i hope i am following the commit-message guidelines now.

(file #48580)

NADER NABIL MANSOUR <nadern96>
Wed 11 Mar 2020 04:45:26 PM UTC, comment #25: 

regarding file #48578:
- I'm not sure what the first section of the patch is coming from (the .hgignore text?) I'm assuming that doesn't need to be there?

- the commit string should mention each m files and c files/functions modified. 
again, see https://wiki.octave.org/Commit_message_guidelines
and see the example mike linked in comment #22

- the helptext needs to note that we're deliberately maintaining the n < 0 matlab incompatibility. (see the end of comment #12)

Nicholas Jankowski <nrjank>
Wed 11 Mar 2020 02:28:48 PM UTC, comment #24: 

I have submitted a new patch that follows all the guidelines and notes in the comment #19 , comment #22 , comment #23 .

(file #48578)

NADER NABIL MANSOUR <nadern96>
Tue 10 Mar 2020 04:29:56 PM UTC, comment #23: 

I would also repeat my comment #9 recommendation to review the wiki guidelines for commit messages:
https://wiki.octave.org/Commit_message_guidelines

Nicholas Jankowski <nrjank>
Mon 09 Mar 2020 11:15:48 PM UTC, comment #22: 

It would be a very good idea for you to learn how to search through Octave's commit log for past changes that are similar to the one you are trying to make, so you can see exactly how we have done things in the past and model your commit after existing practices.

For example, I searched the Mercurial commit history for messages with the key string "compat" that touched the NEWS file, and found this good example that hits all the same points that this bug needs:

https://hg.savannah.gnu.org/hgweb/octave/rev/177be3c01238

Mike Miller <mtmiller>
Project Administrator
Mon 09 Mar 2020 11:09:40 PM UTC, comment #21: 

Yes, that's right, Octave's build system specifically looks for those comment test blocks in source files and extracts them to be run as unit tests. This is documented here: https://octave.org/doc/interpreter/Test-Functions.html

Mike Miller <mtmiller>
Project Administrator
Mon 09 Mar 2020 10:50:43 PM UTC, comment #20: 

what do you mean by " existing test blocks in strfns.cc" , these commented blocks ?

like that :

/*
%!assert (strncmp ("abce", "abc", 3), true)
%!assert (strncmp ("abce", "aBc", 3), false)
*/

NADER NABIL MANSOUR <nadern96>
Mon 09 Mar 2020 07:04:08 PM UTC, comment #19: 

The latest patch submission here includes some suggestions, but still needs a lot of work or a complete rewrite. Here are some specific problems that make the latest patch unusable, there may be other issues, but please address these at the least.

  • Deleted code should not be turned into a comment, just deleted.
  • Any change that affects 'strncmp' should probably be applied equally to 'strncmpi', and include tests for both.
  • Take comment #4 and comment #10 into consideration, this patch completely ignores them.
  • Tests should be added to the already existing test blocks in strfns.cc, not a new test script.
  • Please try to conform to existing writing style when adding new text to docstrings and the NEWS file. There are plenty of examples of the Octave documentation writing style in docstrings and old NEWS files to refer to, use them.
Mike Miller <mtmiller>
Project Administrator
Thu 05 Mar 2020 12:28:51 AM UTC, comment #18: 

i sent @Samarth an email and he told me that he in not going to continue working on that bug

i have submitted a new patch that follow all the guidelines and notes in the comment #17 .

NADER NABIL MANSOUR <nadern96>
Wed 04 Mar 2020 03:18:59 AM UTC, comment #17: 

This patch contains nothing except two lines of text for the function help.

You have yet to include/address:
- the actual code for the bug fix
- any the self tests. 
- any of the details from comment #3, comment #4, comment #5, comment #6, comment #9, comment #10, comment #12
- the other patches make similar changes in strncmpi, yours doesn't address that
- your patch fails to follow commit message guidelines [1] in any way
- it seems maybe you aren't sure how to create a mercurial changeset that captures the full set of changes to fix the bug, since your patches don't include anything related to the function code, or this separate test file you've created insetad of BIST tests in the function code, or changes to the news file.
- you have yet to address the fact that you threw an incomplete patch into a bug report being worked on by 1-2 other people already without any courtesy or interaction with those contributors.

[1] https://wiki.octave.org/Commit_message_guidelines

Nicholas Jankowski <nrjank>
Tue 03 Mar 2020 11:31:52 PM UTC, comment #16: 

i have submitted another patch where i added a note in the help text of the strncmp function for the case : (n<=0)

And adding a self test for the function , check the changes please

NADER NABIL MANSOUR <nadern96>
Tue 03 Mar 2020 03:13:17 AM UTC, comment #15: 

@nadern I see you submitted a patch against this bug without any commentary. Are you working with Samarth or Utkarsh or has this bug just become an open field day for competitive patch submittal?

Your patch seems to only make an edit to the News, mentions a separate unincluded test file rather than creating Built In Self Tests (BISTS), ignores most of the commentary about compatibility from these comments, and doesn't follow Octaves patch submittal guidelines.  Generally it's preferred to have a single patch make all of the changes associated with a bug.

Octave is a community development effort. Learn to work with the community.

Nicholas Jankowski <nrjank>
Thu 27 Feb 2020 07:54:53 PM UTC, comment #14: 

I've been occupied with some stuff lately. @Samarth: Please go ahead with working on the bug

Utkarsh Dhandhania <yoodee>
Thu 27 Feb 2020 07:34:22 PM UTC, comment #13: 

I don't believe there is a general list across all of Octave that documents incompatibilities so no action item there.

Rik <rik5>
Project Administrator
Thu 27 Feb 2020 07:25:41 PM UTC, comment #12: 

@Samarth: ok, it was less than a week since Utkarsh uploaded his patch and indicated he would make the documentation revisions. We wouldn't want people accidentally competing on a bug and wasting each other's time. Perhaps you should reach out to him and verify that he hasn't completed the documentation yet.

@rik: If we're going to deliberately codify an incompatibility with a documented Matlab behavior, is there anywhere else we document known incompatibilities that should be added to the patch? (I thought there was a list.) I would also suggest Samarth or Utkarsh add a "MATLAB Incompatibility: ..." note to the function help since it could cause m-code to fail that does runs on matlab.

Nicholas Jankowski <nrjank>
Thu 27 Feb 2020 06:31:41 PM UTC, comment #11: 

@Nicholas I am not currently working with Utkarsh on this patch. I just saw this patch and since there was no activity on this for few days, I decided to contribute.

I will add the things requested in comment #6 very soon and will also take care of the format for patch comments. I am very sorry for that and it will not repeat again.

@Rik in my patch octave will still give error when n < 0 and I have only changed the functionality for the case n = 0.

Samarth Agarwal <samarth1729>
Thu 27 Feb 2020 06:25:34 PM UTC, comment #10: 

I would like Octave to error out for n < 0 as an indication that they are using the function incorrectly.  The principal thing to change is the N = 0 case.

Agree that documentation, BIST tests, and a note in the NEWS file are also needed.

Rik <rik5>
Project Administrator
Thu 27 Feb 2020 05:02:55 PM UTC, comment #9: 

are you working with Utkarsh on this patch?

As mentioned in comment #5, for matlab compatibility the functions should not produce an error for n < 0 , it should return true.

Also, this patch still doesn't have any updates to the help text, self tests, or NEWS as requested in comment #6.

Last, you should see the wiki page description of recommended format for patch comments [1]

[1] https://wiki.octave.org/Commit_message_guidelines

Nicholas Jankowski <nrjank>
Thu 27 Feb 2020 04:01:07 PM UTC, comment #8: 

I have created a patch which still gives error for n < 0 but returns 1 for n = 0 case. Can someone please review it and tell me what else should be done?

Samarth Agarwal <samarth1729>
Sat 22 Feb 2020 01:56:42 PM UTC, comment #7: 

Okay got it. I realized that the patch I posted is incorrect. I'm sorry. Please ignore it. I'll work on it and post a new file with added self tests and add a note in the help text.

Utkarsh Dhandhania <yoodee>
Sat 22 Feb 2020 01:32:09 PM UTC, comment #6: 

Also, Utkarsh, in addition to some output and error tests that confirm compatible behavior for your changes and the MATLAB documented array behavior, it would be worth adding a note about the n<1 behavior to the help text and a mention of the change in the compatibility  section of the NEWS file

Nicholas Jankowski <nrjank>
Sat 22 Feb 2020 11:42:30 AM UTC, comment #5: 

 I thought it may have been an undocumented  unintended occurrence as well, which is why I went to the Matlab help to check.

The Matlab help [1] for the function specifically calls out n<1 should produce a result of TRUE. I neglected to link the help as reference in comment #2.

My main concern is that the help also describes specific output behaviors for arrays of strings and chars, and i don't know if the function validates for those.

[1] https://www.mathworks.com/help/matlab/ref/strncmp.html

Nicholas Jankowski <nrjank>
Sat 22 Feb 2020 05:00:33 AM UTC, comment #4: 

The use of N < 0 most likely indicates a situation the programmer did not anticipate and I believe Octave should produce an error for this case, regardless of what Matlab does.  In general, Octave validates its inputs more carefully than Matlab in order to prevent garbage in / garbage out calculations.

I feel the same way about N = 0, but there is a better argument to be made there because zero length, i.e., empty strings are real whereas strings with negative length do not exist.

Note that the C library function uses size_t for N which is an unsigned data type.  That suggests requiring N to be in the range [0, MAX_INT] during input validation.  Otherwise, -1 will be mapped to MAX_INT-1 which will not do the right thing at all.

Rik <rik5>
Project Administrator
Sat 22 Feb 2020 04:19:39 AM UTC, comment #3: 

can you add a couple self tests at the end of each to verify this expected behavior? (and to help it not gets lost in a future code change)

Nicholas Jankowski <nrjank>
Sat 22 Feb 2020 01:37:07 AM UTC, comment #2: 

and that is spelled out in the matlab help, so this is a documented compatibility bug

If n is 0, then strncmp always returns 1. By convention, the zeroth character of a character vector or a string scalar is always '', a 0-by-0 character array.

If n is less than 0, then strncmp treats it as 0.

Nicholas Jankowski <nrjank>
Sat 22 Feb 2020 01:34:54 AM UTC, comment #1: 

it appears for Matlab 2019a, it returns 1 for any N<1. 

>> strncmp("abc","def",0)
ans =
  logical
   1
>> strncmp("abc","def",-1)
ans =
  logical
   1
>> strncmp("abc","def",-10)
ans =
  logical
   1

Nicholas Jankowski <nrjank>
Fri 21 Feb 2020 11:44:25 PM UTC, original submission:  

strncmp(str_a, str_b, N) function basically takes 2 strings and a number N and compares the 2 strings up to first N characters. If the first N characters are equal in both strings, the function returns 1, otherwise 0.

Command -

strncmp("abc","def",0)

Output -
error: strncmp: N must be greater than 0

In this example, MATLAB returns 1, which seems reasonable

Same unexpected error in case of the ignore-case version of this function - strncmpi

Utkarsh Dhandhania <yoodee>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #48590:  mypatch.patch added by nadern96 (3KiB - text/x-patch)
file #48585:  mypatch.patch added by nadern96 (3KiB - text/x-patch)
file #48584:  mypatch.patch added by nadern96 (3KiB - text/x-patch)
file #48580:  mypatch.patch added by nadern96 (3KiB - text/x-patch)
file #48578:  mypatch.patch added by nadern96 (7KiB - text/x-patch)
file #48550:  strncmp.patch added by nadern96 (4KiB - text/x-patch)
file #48548:  strncmp.patch added by nadern96 (933B - text/x-patch - i have created a patch in which i solve strncmp problem and make it matlab Compatible , update help , and add self test)
file #48539:  strncmp.patch added by nadern96 (863B - text/x-patch - bug #57879 Solution , i have created a patch in which i solve strncmp problem and make it matlab Compatibility)
file #48509:  bug57879.patch added by samarth1729 (1KiB - text/x-patch - I have created a patch which still gives error for n < 0 but returns 1 for n = 0 case. Can someone please review it and tell me what else should be done)
file #48469:  bug57879patch.patch added by yoodee (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by nadern96 (Updated the item)
  • -email is unavailable- added by samarth1729 (Updated the item)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by yoodee (Submitted the item)
  • -email is unavailable- added by yoodee
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-03-12 nadern96 Attached File- => Added mypatch.patch, #48590
    2020-03-12 nadern96 Attached File- => Added mypatch.patch, #48585
    2020-03-12 nadern96 Attached File- => Added mypatch.patch, #48584
    2020-03-11 nadern96 Attached File- => Added mypatch.patch, #48580
    2020-03-11 nadern96 Attached File- => Added mypatch.patch, #48578
    2020-03-05 nadern96 Attached File- => Added strncmp.patch, #48550
    2020-03-03 nadern96 Attached File- => Added strncmp.patch, #48548
    2020-03-03 nadern96 Attached File- => Added strncmp.patch, #48539
    2020-02-27 samarth1729 Attached File- => Added bug57879.patch, #48509
    2020-02-22 yoodee Attached File- => Added bug57879patch.patch, #48469
    2020-02-22 mtmiller Severity3 - Normal => 2 - Minor
        Item GroupUnexpected Error => Matlab Compatibility
        StatusNone => Confirmed
    2020-02-21 yoodee Carbon-Copy- => Added yoodee

    Back to the top


    Powered by Savane 3.5