bugGNU Octave - Bugs: bug #58617, cell2struct silently truncates...

 
 

bug #58617: cell2struct silently truncates char matrices

Submitter:  Kai Torben Ohlhus <siko1056>
Submitted:  Fri 19 Jun 2020 02:31:55 AM UTC
   
 
Category:  Interpreter Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  siko1056
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 09 Sep 2020 02:54:43 AM UTC, comment #6: 

Thank you jwe for the changeset. 👍

All test cases of my original comment work.

Fixed and closed.

Kai Torben Ohlhus <siko1056>
Group Member
Tue 08 Sep 2020 05:54:05 PM UTC, comment #5: 

The extra return statement was probably left over from when error didn't throw an exception, so we just set error_state and returned.

One issue is definitely bug #49536, that string_value is only returning the first row of a character array and truncating arrays without warning.

RE comment #1, cellstr (['foo';'bar';'baz']) creates a 3x1 cell array with each element containing a 1x3 character array, but {['foo';'bar';'baz']} creates a 1x1 cell array containing a 3x3 character array.

I pushed the following change for the immediate problem with cell2struct and will leave the problem with string_value/cellstr_value to bug #49536.

http://hg.savannah.gnu.org/hgweb/octave/rev/b3770a5f210d

John W. Eaton <jwe>
Group administrator
Mon 22 Jun 2020 05:16:42 AM UTC, comment #4: 

Rik, thank you for going into this.  This surprised me indeed, too.
 

// compile with:
//
//  (clan)g++ -std=c++11 -Wall -Wextra -pedantic

#include <iostream>
#include <memory>
#include <vector>

std::unique_ptr<std::vector<int>> global_stuff;

std::vector<int> do_something () {
  std::vector<int> retval;

  return *global_stuff;

  return retval;
}

int main () {
  global_stuff = std::unique_ptr<std::vector<int>> (new std::vector<int> ({1, 2, 3}));

  std::vector<int> val = do_something ();

  for (auto const& v : val) {
    std::cout << v << ' ' << std::endl;
  }

  return 0;
}


Entirely undetected my garbage in both GCC and clang?!

Anyways, I fixed this
https://hg.savannah.gnu.org/hgweb/octave/rev/bc521cd24922

It might take more time to get into the rest, where to fix this appropriately.

Kai Torben Ohlhus <siko1056>
Group Member
Fri 19 Jun 2020 08:14:04 PM UTC, comment #3: 

There are problems all the way down the line.  The cellstr_value() function that is getting called is in ov-cell.cc.  It contains two return statements which is grammatically incorrect for C++.  Only the first has any effect, and I don't know why there isn't a warning during compilation about this.


Array<std::string>
octave_cell::cellstr_value (void) const
{
  Array<std::string> retval;

  if (! iscellstr ())
    error ("invalid conversion from cell array to array of strings");

  if (cellstr_cache->isempty ())
    *cellstr_cache = matrix.cellstr_value ();

  return *cellstr_cache;

  return retval;
}



Rik <rik5>
Group administrator
Fri 19 Jun 2020 07:49:15 PM UTC, comment #2: 

Taking a look at the code in ov-struct.cc for cell2struct I see


  if (! (args(1).iscellstr () || args(1).is_char_matrix ()))
    error ("cell2struct: FIELDS must be a cell array of strings or a character matrix");

  int dim = 0;

  if (nargin == 3)
    {
      if (! args(2).is_real_scalar ())
        error ("cell2struct: DIM must be a real scalar");

      dim = args(2).int_value () - 1;
    }

  if (dim < 0)
    error ("cell2struct: DIM must be a valid dimension");

  const Cell vals = args(0).cell_value ();
  const Array<std::string> fields = args(1).cellstr_value ();

  octave_idx_type ext = (vals.ndims () > dim ? vals.dims ()(dim) : 1);

  if (ext != fields.numel ())
    error ("cell2struct: number of FIELDS does not match dimension");


So, the problem is a little deeper.  Have to look at cellstr_value() and see how it operates.


Rik <rik5>
Group administrator
Fri 19 Jun 2020 07:12:35 PM UTC, comment #1: 

This is most likely because string_value() member function of octave_value class takes just the first row from a char array().

Long term fix is probably to fix the way this internal function works.  See bug #49536 reported by Mike Miller.

As an aside, if you make this a cellstr then you get an error message which makes sense


octave:3> cell2struct({[1,2]}, cellstr (['foo';'bar';'baz']))
error: cell2struct: number of FIELDS does not match dimension



Rik <rik5>
Group administrator
Fri 19 Jun 2020 02:31:55 AM UTC, original submission:  

A subtle truncation happens when calling


containers.Map(['foo';'bar';'baz'], [1,2])


https://lists.gnu.org/archive/html/octave-maintainers/2020-06/msg00088.html

The cause is that cell2struct allows the following:

Octave 6.0.1:


>> cell2struct({[1,2]}, {['foo','bar','baz']})
ans =

  scalar structure containing the fields:

    foobarbaz =

       1   2


>> cell2struct({[1,2]}, {['foo';'bar';'baz']})
ans =

  scalar structure containing the fields:

    foo =

       1   2


while Matlab R2020b does not:


>> cell2struct({[1,2]}, {['foo','bar','baz']})

ans =

  struct with fields:

    foobarbaz: [1 2]

>> cell2struct({[1,2]}, {['foo';'bar';'baz']})
Error using cell2struct
Field name must be a character vector or string scalar.


I agree to Matlab's behavior in this case, because my valuable key is truncated, without a warning.

Kai Torben Ohlhus <siko1056>
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

 

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

    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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-09-09 siko1056 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-09-08 jwe StatusConfirmed Ready For Test
    2020-06-22 siko1056 Assigned toNone siko1056
    2020-06-19 rik5 Dependencies- Depends on bugs #49536
    2020-06-19 rik5 StatusNone Confirmed
        Operating SystemBSD Any
    2020-06-19 siko1056 Carbon-Copy- Added abdallah elshamy <abdallah.k.elshamy@gmail.com>

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code