bugGNU Octave - Bugs: bug #48726, caseless_str overloads compare and...

 
 

bug #48726: caseless_str overloads compare and inverts meaning

Submitter:  Carnë Draug <carandraug>
Submitted:  Tue 09 Aug 2016 02:28:56 AM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  Confirmed Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 13 Aug 2016 02:55:29 PM UTC, comment #10: 

I went with the mentioned proposed and added other templates that would compare a string or Array with a char[].  This allows to use 'strcmp (some_string, "foo")' without creating a string for "foo".

A possible improvement would be to make the functions with two template parameters instead of one so that one could compare Array<char> with std::string.  This is probably very close to work but I didn't do it because I don't think we have a case for it.

I made use of the new functions in a few places but didn't touch the graphics and text engine.  I saw caseless_str calling the find method which caseless_str does not overload.  But it does overload the operator < (and not the operator > or ==) so I'm guessing it relies on find of the parent class use only the < operator. Or maybe it meant the find to be case sensitive?  I'm not sure, so didn't touch those pieces.

Carnë Draug <carandraug>
Group Member
Fri 12 Aug 2016 12:06:24 AM UTC, comment #9: 

My proposal adds the strcmp and strcmpi that you would expect in Octave.

We could add operators to caseless_str but as you already found out, that is tricky and I think it would be easy to accidentally call the operators for the parent classes. Also, subclassing from std:: is never recommended (although the main reason is lack of virtual destructor which we wouldn't need in this case).

I only need a size_type in Array to make it work, so I sent a message to maintainers list about it.

Carnë Draug <carandraug>
Group Member
Thu 11 Aug 2016 05:57:22 PM UTC, comment #8: 

I've never liked the use of the .compare() function.  Mainly, because I think the code is cleaner and more concise if one can use operators like '==' and '!='.  Those are defined for regular string objects, but not for caseless_str objects.  I tried a while ago to actually overload those operators, but ran in to difficulties and abandoned the project.

I think anything you want to do to improve this is probably alright.

Rik <rik5>
Group administrator
Wed 10 Aug 2016 11:23:45 PM UTC, comment #7: 

I managed to make it even simpler.  Instead of linking to commits, I guess it's bettter to link to a bookmark https://bitbucket.org/carandraug/octave/commits/branch/liboctave-strcmp in case I want to make more changes again.

Carnë Draug <carandraug>
Group Member
Wed 10 Aug 2016 07:23:52 PM UTC, comment #6: 

What about this which replaces the code in stfns? https://bitbucket.org/carandraug/octave/commits/64182944beeaa0d74cdbccfd47ad30baf31ab20c

While doing this, I found would be useful to have Array typedef octave_idx_type as size_type like the STL containers do.  That will make it easier to write templates that can handle both. I just worked around it by adding another template parameter but it could be avoided.  What do you think?

Carnë Draug <carandraug>
Group Member
Wed 10 Aug 2016 12:53:27 AM UTC, comment #5: 


> we should consider libinterp/corefcn/strfns.cc, where the octave functions are implemented. There is a similar implementation like in comment #2 in line 686. I think we can avoid some code duplication here.


Absolutely. I noted the same. Just checking and waiting if someone will oppose to have strcmp and strcmpi in a new namespace named octave::string before working on it.

Carnë Draug <carandraug>
Group Member
Tue 09 Aug 2016 10:30:09 PM UTC, comment #4: 

Your code of comment #3 is a much clearer solution than caseless_str. I vote for adding these functions to liboctave for general usage.

But before it gets committed, we should consider libinterp/corefcn/strfns.cc, where the octave functions are implemented. There is a similar implementation like in comment #2 in line 686. I think we can avoid some code duplication here.

Kai Torben Ohlhus <siko1056>
Group Member
Tue 09 Aug 2016 04:52:04 PM UTC, comment #3: 

While caseless_str is mostly used in graphics.h, I don't think we should limit it there. That we don't have something for this in liboctave is the main reason why oct functions have string options case sensitive while the rest of Octave is case insensitive.

So what if we add functions for this to the octave namespace. octave::string sounds good to me. Also, this functions would have the semantics of Octave's strcmp and strcmpi and only return a bool. I don't think there's an issue of having the same name as std::strcmp. That's what namespaces are for, and I think it's more important that we match the behaviour of Octave than C, it seems less surprising to me.

Something like this https://bitbucket.org/carandraug/octave/commits/5c04acadf38c8ed08841c5cf23704c688ca724d5

strfns.cc could then be modified to make use of these.

Carnë Draug <carandraug>
Group Member
Tue 09 Aug 2016 12:19:22 PM UTC, comment #2: 

I agree with you Carnë. The only function used according to Doxygen is caseless_str::compare() and this in turn is mainly used in libinterp/corefcn/graphics.cc. Maybe we can establish a short inline function like "match_case_insensitive()" below there? This should not confuse with the semantics of std::string::compare() and is simple, stupid, short. Do we have a string utility header as alternative placement? Anyway, this function, as like as the existing one, works only for non-Unicode characters.


#include <iostream>
#include <string>
#include <algorithm>

bool match_case_insensitive(std::string const& a, std::string const& b) {
  if (a.length() == b.length()) {
    return std::equal(a.begin(), a.end(), b.begin(),
      [](unsigned char ca, unsigned char cb) {
        return std::tolower(ca) == std::tolower(cb); });
  } else {
    return false;
  }
}

int main (){
  std::string s1 = "hallo";
  std::string s2 = "hallo";
  std::string s3 = "hAlLo";
  std::cout << s1.compare(s2) << " "
            << match_case_insensitive(s1, s2) << " "
            << s1.compare(s2) << std::endl;
  std::cout << s1.compare(s3) << " "
            << match_case_insensitive(s1, s3) << " "
            << s1.compare(s3) << std::endl;
  return 0;
}


Source: https://stackoverflow.com/questions/23943728/case-insensitive-standard-string-comparison-in-c

Kai Torben Ohlhus <siko1056>
Group Member
Tue 09 Aug 2016 03:31:19 AM UTC, comment #1: 

I think this class is meant as helper for input check from the interpreter. And is meant to be compared against hardcoded strings which we know whether they are upper or lower case. Because of that, having the existing compare method, lowercase both strings could be avoided.

This https://bitbucket.org/carandraug/octave/commits/cb281077b99be71ee87fe80e8e3c5fab93fc010a adds two methods, compare_lower and compare_upper for such cases. This methods return an int, not a bool, with the same semantycs one would expect from std::string::compare().

I am also thinking that they might be better implemented as free functions on the octave namespace.

Carnë Draug <carandraug>
Group Member
Tue 09 Aug 2016 02:28:56 AM UTC, original submission:  

liboctave has a caseless_str class which subclasses std::string.  It overloads the compare method so that it compares both string in lowercase.

The issue is that the compare method returns a zero when strings are equal. The overloaded methods returns true in that case, the opposite what one would expect. This comes a bit of a surprise.

Also, the == and != operators have not been overloaded. I'm not sure they should be, this sounds like trouble.

Carnë Draug <carandraug>
Group Member

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by carandraug (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-09 siko1056 CategoryNone Libraries
        Item GroupNone Other
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code