bugGNU Octave - Bugs: bug #62802, "methods classdef" shows...

 
 

bug #62802: "methods classdef" shows some methods twice for mixed classdef/@class classes

Submitter:  Colin Macdonald <cbm>
Submitted:  Sun 24 Jul 2022 04:38:09 AM UTC
   
 
Category:  Classdef Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Inaccurate Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 7.2.0 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 17 Oct 2022 11:08:25 AM UTC, comment #13: 

CI passed.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Mon 17 Oct 2022 07:32:22 AM UTC, comment #12: 

I added a BIST for this here to catch a possible regression:
https://hg.savannah.gnu.org/hgweb/octave/rev/b01e8e322838

Markus Mützel <mmuetzel>
Group administrator
Wed 03 Aug 2022 10:47:55 PM UTC, comment #11: 

I fixed this on stable in this changeset http://hg.savannah.gnu.org/hgweb/octave/rev/fb1c7c8a030b.

I took the less compute-intensive approach of using get_method_map when the object is a classdef object or load_path.methods when it is a legacy @CLASS rather than using both and then uniquifying the result.

Marking as Ready for Test.

Rik <rik5>
Group administrator
Mon 01 Aug 2022 07:26:16 AM UTC, comment #10: 

I attached a changeset that is essentially what we have been discussing below -- a call to string_vector::sort, and some comments to explain the call to sort.

(file #53496)

Michael Bagherpour <bagelboy>
Mon 01 Aug 2022 04:11:30 AM UTC, comment #9: 

I'm not sure whether it will add any new information beyond what has been covered here, but there was also some discussion about the methods function in bug #42510.

John W. Eaton <jwe>
Group administrator
Mon 01 Aug 2022 03:57:48 AM UTC, comment #8: 

Near the bottom of the page for each bug report there is a section to attach files.  You can attach a changeset there.  It's best to also submit a comment so that people will notice that a new file has been attached.

John W. Eaton <jwe>
Group administrator
Mon 01 Aug 2022 03:50:16 AM UTC, comment #7: 

Speaking of patches, how do we submit a patch? The patches page in savannah says to only submit patches through that page if it's a new feature, and to use the bug tracker for bug fixes. (https://savannah.gnu.org/patch/?group=octave&func=additem)
Maybe I'm just missing it, but I don't see anywhere to submit a patch in the bug tracker?

Sorry for nooby questions -- I'm still a noob, but working on it :)

Michael Bagherpour <bagelboy>
Sun 31 Jul 2022 08:41:10 PM UTC, comment #6: 

I don't know anything about this code, but maybe that wallpapers over something else?  I guess the question is whether the code should be:

if ~is_classdef:  # (pseudocode)
    // The following will also find methods for legacy @CLASS objects.
   load_path& lp = interp.get_load_path ();

   sv.append (lp.methods (class_name));


Personally, I'd be fine with adding a comment:

   // The following will also find methods for legacy @CLASS objects.
+  // this may count methods twice for classdef classes, Bug #62802
   load_path& lp = interp.get_load_path ();

   sv.append (lp.methods (class_name));
+  sv.sort(true);

   return ovl (Cell (sv));
 }


IMHO, returning sorted methods sounds good for deterministic testing and pleasant user experience.  I doubt there are classes where the performance hit of sorting would be significant.

I'm happy to test a patch!

Colin Macdonald <cbm>
Sat 30 Jul 2022 08:42:18 PM UTC, comment #5: 

It seems like we can sort the methods and then call uniq. In fact, the sort method itself does call uniq if passed in a flag:


string_vector&
string_vector::sort (bool make_uniq)
{
  // Don't use Array<std::string>::sort () to allow sorting in place.
  octave_sort<std::string> lsort;
  lsort.sort (m_data.fortran_vec (), numel ());

  if (make_uniq)
    uniq ();

  return *this;
}


Modifying the code after line 760 in https://hg.savannah.gnu.org/hgweb/octave/file/e37e46ef0505/libinterp/octave-value/ov-classdef.cc to the following works for me:


   // The following will also find methods for legacy @CLASS objects.
   load_path& lp = interp.get_load_path ();

   sv.append (lp.methods (class_name));
+  sv.sort(true);

   return ovl (Cell (sv));
 }


Does that seem right? Any reason not to do this?

Michael Bagherpour <bagelboy>
Sat 30 Jul 2022 05:08:02 PM UTC, comment #4: 

I tried commenting out as:

   // The following will also find methods for legacy @CLASS objects.
-  load_path& lp = interp.get_load_path ();
+  //load_path& lp = interp.get_load_path ();

-  sv.append (lp.methods (class_name));
+  //sv.append (lp.methods (class_name));

This does fix the duplicated methods on my MWE, but predictably it breaks old-style ("legacy" here) `@class`.

Colin Macdonald <cbm>
Sat 30 Jul 2022 04:16:33 PM UTC, comment #3: 

uniq: presumably for O(N) efficiency?  Seems like there should be a comment somewhere!

Thanks for tracking down (so far) the duplicated methods.  The MWE does indeed have mix of classdef with `@test_classdef/amethod.m`.

Colin Macdonald <cbm>
Sat 30 Jul 2022 01:06:15 PM UTC, comment #2: 

If I understand the implementation correctly, the string_vector must be sorted (or the duplicate items must be next to each other) for `string_vector::uniq()` to actually remove all duplicate strings.
https://hg.savannah.gnu.org/hgweb/octave/file/e37e46ef0505/liboctave/util/str-vec.cc#l88

string_vector&
string_vector::uniq (void)
{
  octave_idx_type len = numel ();

  if (len > 0)
    {
      octave_idx_type k = 0;

      for (octave_idx_type i = 1; i < len; i++)
        if (elem (i) != elem (k))
          if (++k != i)
            elem (k) = elem (i);

      if (len != ++k)
        resize (k);
    }

  return *this;
}


Markus Mützel <mmuetzel>
Group administrator
Sat 30 Jul 2022 08:22:14 AM UTC, comment #1: 

Stepping through the code at libinterp/octave-value/ov-classdef.cc with gdb (image attached), it looks like the files in the class are added first in the loop at line 754, and then again at line 767.
The comment at line 764 says that this is meant to deal with legacy @class methods. I don't know the full context, but I wonder if legacy @class methods still need to be handled as a special case.
If it does need to handle the special case, I think we should be able to use string_vector::uniq() to eliminate the duplicates. However, in brief testing it seems like uniq() might also have a bug -- it's not working for me.


Anonymous
Sun 24 Jul 2022 04:38:09 AM UTC, original submission:  

Today's tip.  Can also reproduce in 7.1.0 and 6.4.0 and I think in the other 6.X.Y.

MWE: see attached `test_classdef.m` and `amethod.m` place both files in a directory called `@test_classdef`.  Or find the same class in Doctest package `test/@test_classdef`

Then do:


>> methods test_classdef
Methods for class test_classdef:
amethod        disp           test_classdef  amethod        test_classdef


We note that "amethod" appears twice.  So does "test_classdef".  But "disp" appears only once.  It seems that external mfile methods are duplicated and the ctor is but not other methods.  But I have not extensively experimented yet.

Note: I think it is correct for the ctor to appear in the list (once) b/c the ctor can have different help than the class itself (another issue about that to follow).

Colin Macdonald <cbm>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #53496:  proposed_changeset_62802 added by bagelboy (909B - application/octet-stream)
file #53467:  test_classdef.m added by cbm (939B - text/x-objcsrc - place both in @test_classdef folder)
file #53468:  amethod.m added by cbm (93B - text/x-objcsrc - place both in @test_classdef folder)

 

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 jwe (Posted a comment)
  • -email is unavailable- added by bagelboy (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by cbm (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-10-17 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2022-08-03 rik5 Item GroupNone Inaccurate Result
        StatusNone Ready For Test
        Releasedev 7.2.0
        Summary&quot;methods classdef&quot; shows some methods twice "methods classdef" shows some methods twice for mixed classdef/@class classes
    2022-08-01 bagelboy Attached File- Added proposed_changeset_62802, #53496
    2022-07-30 None Attached File- Added Screenshot@from@2022-07-30@00-25-27.png, #53488
    2022-07-24 cbm Attached File- Added test_classdef.m, #53467
        Attached File- Added amethod.m, #53468

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code