bugGNU Octave - Bugs: bug #55961, properties function does not...

 
 

bug #55961: properties function does not preserve order

Submitter:  John W. Eaton <jwe>
Submitted:  Wed 20 Mar 2019 03:47:31 AM UTC
   
 
Category:  Classdef Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Ready For Test Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  10.1.0 (current default)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 14 Jul 2024 04:11:24 PM UTC, comment #7: 

No feedback during more than a week. So, I'm assuming it is ok to change the API for these functions without any deprecation period.

I pushed your patch (with some additional changes) here:
https://hg.savannah.gnu.org/hgweb/octave/rev/ef4511ee5b87

Sorry again for taking so long for getting this reviewed and merged.

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Fri 05 Jul 2024 06:09:44 PM UTC, comment #6: 

Is there even a way to keep two member functions of a class that only differ in the type of their return value?
E.g., would it be possible to have two member functions like the following:

std::map<std::string, cdef_property>
cdef_class::get_property_map (int mode);
std::map<property_key, cdef_property>
cdef_class::get_property_map (int mode);


I tried to do that using templates, i.e.:

    template <typename T>
    T get_property_map (int mode);


And then deprecate one specialization:

template
std::map<property_key, cdef_property>
cdef_class::get_property_map (int mode);

template
OCTAVE_DEPRECATED (10, "std::map<std::string, cdef_property> is deprecated, use std::map<property_key, cdef_property> instead")
std::map<std::string, cdef_property>
cdef_class::get_property_map (int mode);


But automatic template type deduction doesn't seem to work with that. So, downstream code would need to pass the template argument explicitly. I.e., they would need to change code like this:

std::map<std::string, cdef_property> props;
props = cdef_class::get_property_map (mode);


to this:

std::map<std::string, cdef_property> props;
props = cdef_class::get_property_map<std::map<std::string, cdef_property>> (mode);


Just to change it again to finally this (potentially including other necessary changes):

std::map<property_key, cdef_property> props;
props = cdef_class::get_property_map (mode);


If downstream actually uses that function, it would probably be just as good to skip the intermediate step and directly use the final version.

@maintainters: Is there a better option to deprecate functions that changed the type of their output arguments? Or is it ok to skip deprecation in this case?

Markus Mützel <mmuetzel>
Group administrator
Sun 30 Jun 2024 03:12:19 PM UTC, comment #5: 

@ies: Sorry for taking so long to finally review your patch. It's now more than 5 years since you originally submitted it. And - of course - it no longer applies on a current head.

The attached patch is rebased on a current head. (Almost every line had to be changed manually...)

Afaict, `make check` still passes. (But I have a lot of local changes that cause some unrelated test failures. I hope I didn't miss relevant errors.)
It would probably be nice to add (at least one) BIST to make sure this change is working as expected with inheritance and doesn't regress in the future. The classes from comment #2 would probably work.

The proposed changes would modify the API in a non-backwards compatible way. I don't know if anyone is using the affected functions and typedefs though. So, this might not be an issue when it comes to existing code.
Usually, we deprecate existing functions for 2 major versions before eventually removing them. We probably could do the same here. I'm not sure this is worth the effort in this particular case though.

@maintainers: Would it be ok to change the C++ API in this case without keeping backwards-compatible deprecated functions and typedefs? (I don't know how typedefs could be deprecated though...)


(file #56214)

Markus Mützel <mmuetzel>
Group administrator
Wed 07 Jul 2021 11:17:08 AM UTC, comment #4: 

The patch has been submitted more than two years ago. Objects with tons of properties become really hard to read if they are in alphabetical order, and the behavior of objects should match that of structures (which have their members printed in the order in which they were defined).

Can someone verify the patch and merge into the next release?

Kind Regards.

Anonymous
Mon 10 Jun 2019 12:38:45 PM UTC, comment #3: 

I have just attached a patch in file 'suggested_fix_for_bug_55961_1.patch'.

Octave (built from the most recent sources) now returns:


octave:2> properties (prop_tester_1)
properties for class prop_tester_1:

  ff_from_1
  dd_from_1
  a_from_1
  ee_from_1
  b_from_1
  c_from_1
  cc_from_1
  bb_from_1

octave:3> properties (prop_tester_2)
properties for class prop_tester_2:

  f_from_2
  d_from_2
  e_from_2
  ff_from_1
  dd_from_1
  a_from_1
  ee_from_1
  b_from_1
  c_from_1
  cc_from_1
  bb_from_1

octave:4> properties (prop_tester_3)
properties for class prop_tester_3:

  ccc_from_3
  bbb_from_3
  aaa_from_3
  eee_from_3
  f_from_2
  d_from_2
  e_from_2
  ff_from_1
  dd_from_1
  a_from_1
  ee_from_1
  b_from_1
  c_from_1
  cc_from_1
  bb_from_1


Just a not about 'duplicates': Matlab will not allow a classdef class to 'overload' a property already found in an ancestor. Any attempt to do such a thing triggers an error message along the lines of:


Error using prop_tester_3
Cannot define property 'e_from_2' in class 'prop_tester_3' because the property has already been defined in the superclass
'prop_tester_2'.


However, Octave allows this, and the existing code was careful not to return the same property twice. I preserved this behaviour  in the sense that there are no duplicates in the returned property list, and a property is returned when it is first encountered while exploring the ancestor list (from the considered child to the most remote ancestor).

ImadES <ies>
Mon 10 Jun 2019 12:27:55 PM UTC, comment #2: 

Hi,


I think attention should also be paid to cases involving inheritance.

Additional examples

Here are two such cases, based on a common ancestor:


classdef prop_tester_1

  properties
    ff_from_1
    dd_from_1
    a_from_1
    ee_from_1
    b_from_1
    c_from_1
    cc_from_1
    bb_from_1
  end

end
classdef prop_tester_2 < prop_tester_1

  properties
    f_from_2
    d_from_2
    e_from_2
  end

end
classdef prop_tester_3 < prop_tester_2

  properties
    ccc_from_3
    bbb_from_3
    aaa_from_3
    eee_from_3
  end

end


Results with Matlab R2019a Update 2


>> properties(prop_tester_1)

Properties for class prop_tester_1:

    ff_from_1
    dd_from_1
    a_from_1
    ee_from_1
    b_from_1
    c_from_1
    cc_from_1
    bb_from_1

>> properties(prop_tester_2)

Properties for class prop_tester_2:

    f_from_2
    d_from_2
    e_from_2
    ff_from_1
    dd_from_1
    a_from_1
    ee_from_1
    b_from_1
    c_from_1
    cc_from_1
    bb_from_1

>> properties(prop_tester_3)

Properties for class prop_tester_3:

    ccc_from_3
    bbb_from_3
    aaa_from_3
    eee_from_3
    f_from_2
    d_from_2
    e_from_2
    ff_from_1
    dd_from_1
    a_from_1
    ee_from_1
    b_from_1
    c_from_1
    cc_from_1
    bb_from_1


Notice how the ancestor are looped over, from the most recent to the furthest.

ImadES <ies>
Mon 20 May 2019 09:46:13 PM UTC, comment #1: 

Yes, verified for Matlab R2019a (update 1):


classdef prop_tester
  properties
    a
    b
    c
    cc
    bb
    dd
  end
end

>> properties (prop_tester)

Properties for class prop_tester:

    a
    b
    c
    cc
    bb
    dd


Kai Torben Ohlhus <siko1056>
Group Member
Wed 20 Mar 2019 03:47:31 AM UTC, original submission:  

The properties function does not return the property names in the same order that they are listed in the classdef definition.  I believe this is needed for Matlab compatibility (could someone verify?).

John W. Eaton <jwe>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #56214:  bug55961-classdef-properties-order-v2.patch added by mmuetzel (12KiB - application/octet-stream)
file #47064:  suggested_fix_for_bug_55961_1.patch added by ies (9KiB - text/x-patch - Attaching a suggested patch which fixes the properties lost order issue)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by nrjank (Updated the item)
  • -email is unavailable- added by ies (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by jwe (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-07-14 mmuetzel StatusPatch Submitted Ready For Test
    2024-06-30 mmuetzel Attached File- Added bug55961-classdef-properties-order-v2.patch, #56214
        CategoryOctave Function Classdef
        Planned ReleaseNone 10.1.0 (current default)
    2021-09-08 nrjank StatusNone Patch Submitted
    2019-06-10 ies Attached File- Added suggested_fix_for_bug_55961_1.patch, #47064

    Back to the top

    Powered by Savane 3.13-3e34.
    Corresponding source code