[bug #67845] classdef constructors do not support multiple arguments

Please use the bug tracker to post updates to a bug report.  The mailing list is intended as a read-only notification stream.  Info posted to this mailing list address won't appear in the tracker database where it is most useful.

Follow-up Comment #4, bug #67845 (group octave):

Hi @fatyoshi17. I took the time to review this patch. It fixed the example
given in comment #0. However, you were a ways off from understanding the
technicalities of this bug. That's not unexpected, since this is a very hard
bug to troubleshoot, and I don't think it's a good one for a newcomer to the
Octave codebase. I went ahead and attached my own patch that resolves this
bug; it is substantially different from yours. I hope you take a look at it
and try to understand why I chose the changes I made.

Before I get into the technical details, here are some of the things that any
successful patch needs:

Your patch needs to have a commit message that follows the guidelines over at
https://wiki.octave.org/Commit_message_guidelines.

You usually need more tests than one. It would have helped you catch some
issues with your patch. Also, classdefs should be in their own separate file
and not defined inside a string.

There is some obvious AI-heavy usage in your patch. The biggest thing that
stands out is the use of an em-dash, which is a non-ASCII character. It is
difficult to unintentionally type a non-ASCII character into a text
editor/IDE. To make my (personal) stance on AI clear: it's not a problem to
use it, but the output should be well-thought out and (mostly)
indistinguishable from what a motivated human can do.

Going on to the architectural problems with this patch: `meta_subsref` is a
method for handling a `subsref` call to a class. That means once a classdef
constructor call is detected, it should immediately call the
`construct_object` method in `cdef_class` and let it handle the specifics. The
logic behind classdef construction should not be in this method specifically.

The consequence of putting all this logic in `meta_subsref` is that parent
constructors are not properly called. Let me explain with an example. Take two
classdefs, `Child` and `Parent`.


classdef Child < Parent
  methods
    function [obj, val] = Child ()
      val = 1;
    end
  end
end



classdef Parent
  properties
    parent_value
  end
  methods
    function obj = Parent ()
      obj.parent_value = 2;
    end
  end
end


If we instantiate `Child` normally, then we should expect to see


>> c = Child ()
c =

  1x1 Child object with properties:

      parent_value: 2



which totally works with your patch. This is because the Child constructor
implicitly calls the Parent superclass constructor, which sets the
`parent_value` to be 2.

And if we do the same instantiation but grab the second return value, we
should get the same Child object:


>> [c, val] = Child ()
c =

  1x1 Child object with properties:

      parent_value: 2

val = 1


However, your patch ended up producing:


>> [c, val] = Child ()
c =

  1x1 Child object with properties:

      parent_value: []

val = 1


Notice that the parent_value value is empty. What happened was that the
multi-output constructor path didn't properly call the superclass constructor
(the `Parent` constructor) in your patch. It's a very subtle bug, but those
are usually the worst kind of bug.

If you have any questions about the architecture of the patch, feel free to
ask them. But I'll suggest again to work on a simpler bug for now.



(file #58349)

    _______________________________________________________

Additional Item Attachment:

Name: bug67845.patch                 Size: 20KiB
    <https://file.savannah.gnu.org/file/bug67845.patch?file_id=58349>


    AGPL NOTICE

These attachments are served by Savane. You can download the corresponding
source code of Savane at
https://savannah.gnu.org/source/savane-5479f0ac3e1f014845fd281c379bc3ccb7a72723.tar.gz


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?67845>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQQk97aszIMMAvLLwm6qLAuaBUf3TgUCabJRCwAKCRCqLAuaBUf3
TtP/AP0aPrLN2h7mxDAN+/nuACINzkKv7RQOSArHlGYN9eRKVQEA3XCNa0fRST2G
q+Ez9FM4Z9wqtC85Rw8Lo475IbF8Egc=
=58Sx
-----END PGP SIGNATURE-----