Fri 11 Aug 2017 04:29:06 AM UTC, comment #14:
I checked, and the original code that motivated the report now runs correctly. Marking as fixed and closing report.
|
Thu 10 Aug 2017 09:45:47 PM UTC, comment #13:
I'm sorry about that. I know that Octave is 2 whitespace and the reasoning behind it.
|
Thu 10 Aug 2017 09:39:43 PM UTC, comment #12:
I pushed the changeset here:
http://hg.savannah.gnu.org/hgweb/octave/rev/5352f632859b
For the future, we try to avoid TAB characters in the Octave sources. Can you configure your editor to use spaces instead of TABs when editing Octave sources?
Thanks.
Meanwhile, I also noticed that my changeset to add the predicates for classdef_superclass_ref objects included an unintended change. I undid that in another changeset.
|
Thu 10 Aug 2017 08:21:50 PM UTC, comment #11:
So I screwed up when doing my cset. I deleted by mistake:
Sorry about that.
I am attaching a cset with the correction.
(file #41475)
|
Thu 10 Aug 2017 05:07:07 PM UTC, comment #10:
I second that. Virtual functions usually are implemented as a jump table so while there is one level of pointer indirection, its still very efficient. On the other hand, dynamic_cast requires the runtime environment to do whatever is necessary to make sure that the object to be converted results in a valid object of the cast type. This might mean initializing new member variables, replacing a virtual function with a new implementation, etc. The performance penalty is not well-bounded.
|
Thu 10 Aug 2017 04:33:43 PM UTC, comment #9:
The predicates also involve virtual function calls so there are performance considerations there too. But I'd just generally prefer to avoid casts. Or to refactor so that directly inquiring about types isn't necessary. But for now, let's stick with the current structure of the code if we can. I'd of course be happy to make it all simpler, but that might not be possible.
|
Thu 10 Aug 2017 04:31:50 PM UTC, comment #8:
Maybe I was wrong. I'll look at it in a couple hours.
|
Thu 10 Aug 2017 04:30:17 PM UTC, comment #7:
Piotr: That doesn't seem to fix the problem or I misunderstood the change you were suggesting.
I checked in the following changeset to add predicates for classdef_superclass_ref objects:
http://hg.savannah.gnu.org/hgweb/octave/rev/d56c18dc1373
|
Thu 10 Aug 2017 04:16:08 PM UTC, comment #6:
Roger that.
|
Thu 10 Aug 2017 04:15:13 PM UTC, comment #5:
Given the frequency that statements are evaluated, I would also be concerned with any cast which might impair performance.
|
Thu 10 Aug 2017 04:06:41 PM UTC, comment #4:
Rik: I added a test:
http://hg.savannah.gnu.org/hgweb/octave/rev/e660b93d51dc
Piotr: I generally prefer to avoid casts if possible.
|
Thu 10 Aug 2017 03:59:13 PM UTC, comment #3:
About this bug: I figure (and haven't checked this) that all that is needed is a is_classdef_superclass_ref () check at here
Should that check be on octave_value level, or octave_function level?
The reason I'm asking is that octave_value has so many is_... checks that I was wondering if it would not be enough to put it on octave_function level and then dynamic cast to it to check. On the other hand, you get a slight performance hit when you do that. Any thoughts?
|
Thu 10 Aug 2017 03:26:54 PM UTC, comment #2:
I also propose adding the following BIST test somewhere so this doesn't keep happening.
|
Thu 10 Aug 2017 02:56:50 PM UTC, comment #1:
This problem was introduced when I refactored the evaluator. Then it was fixed here:
and reintroduced here:
Adding Piotr to the notification list.
|
Thu 10 Aug 2017 03:17:30 AM UTC, original submission:
Without parentheses '()' to help the interpreter, Octave issues an invalid index error.
Example Code:
But adding '()' makes the code work correctly
|