Thu 21 Apr 2011 09:53:13 PM UTC, comment #21:
OK, I updated the log message and committed the change to the default branch. I'm marking this report as fixed and closing it. If there are other problems, please open a new bug report.
|
Thu 21 Apr 2011 09:40:38 PM UTC, comment #20:
Yay! I think we're finally done with this patch.
The only thing I would change would be to move the (bug #xxxxx) reference into the first summary line of the commit message.
'hg log' only shows the first line and it is convenient to use 'hg log | grep "bug #"' to find commits for bug reports.
|
Thu 21 Apr 2011 08:42:20 PM UTC, comment #19:
Before committing, I need to update the commit message to mention the lexer changes. If this changeset is OK, let me know and I'll commit it.
|
Thu 21 Apr 2011 08:40:29 PM UTC, comment #18:
I think I've fixed the transpose problem (it looks like it was a typo in the precedence table).
I also fixed the a--' problem. This was a lexer bug. It was not allowing ' to be recognized as a transpose operator after -- or ++. Now it is. But I think your test was not quite right, so I fixed it. Instead of
I changed it to
My current version of the changeset is attached.
Either that, or I think it needs to reinitialize a before doing the --' operation.
(file #23286)
|
Thu 21 Apr 2011 07:14:53 PM UTC, comment #17:
I re-wrote the parser tests in test_parser.m to conform to the new precedence hierarchy. I'm attaching a diff to this bug report which is your parser changes + my test code.
There are a few failing tests that need addressing.
1) exponent and transpose (the original reporter's issue) is back again. These two operators are at the same level of precedence and should evaluate from left-to-right.
2) postfix operator and Hermitian conjugate don't work together. Ordinary transpose does (a--.')
(file #23284)
|
Wed 20 Apr 2011 02:58:27 AM UTC, comment #16:
OK, I'm attaching another attempt. With this, it passes all the tests and the additional one you show below.
(file #23278)
|
Wed 20 Apr 2011 02:35:40 AM UTC, comment #15:
I started trying to write new parser tests against the new documented behavior and came across this:
I think the 3.4 behavior is correct since a++ should return 1 to the current expression being evaluated while incrementing the variable a only after the entire expression has been evaluated.
|
Tue 19 Apr 2011 10:39:24 PM UTC, comment #14:
I'm attaching the latest changes. I think this is now a relatively minor change.
The precedence of | has been less than & (and likewise for || and &&) for a long time now. This is consistent with C and C++, I think, and compatible with Matlab, so there is actually no change there.
I deleted the test for ---a and fixed the one for a^b^c.
The shift operators have not been defined in Octave's scripting language for a long time now, but the parser still recognizes them so it is possible that someone is using them with a user-defined type. But it seems unlikely enough that I'm willing to just remove them and see if anyone complains. I'll do that in a separate changeset.
All your parser tests now pass with my current version of the changeset applied.
I think the docs are now correct, and I reordered the table to have highest precedence at the top as you suggested.
(file #23277)
|
Tue 19 Apr 2011 06:29:36 PM UTC, comment #13:
>> I also fixed the test for exponentiation. Matlab docs specifically note that exponentiation is handled left to right. That's odd to me, but I don't see a compelling reason to do things differently.
Agreed. It's simpler if is just another left-to-right operator. This sentence in expr.txi should be changed to eliminate the reference to exponentiation: "When operators of equal precedence are used together, the leftmost operator groups first, except for the assignment and exponentiation operators, ..."
>>I also changed the precedence levels of the || and &&, and | and & operators to match Matlab, with the OR operators having lower precedence than the AND operators. Should we try to warn about this change in 3.6?
I don't see the logic in having '|' and '&' at different precedence levels and I don't believe we need to follow Matlab down this path. But if we do, we will need some tests in test/test_parser.m to check that the newly inserted levels are working correctly. And yes, we will definitely need to post something to NEWS since a|b&c will now evaluate as a|(b&c) which is the opposite of the existing (a|b)&c interpretation. Or more likely, an issue will result from using the short-circuit operators. I would expect a || b && c to first check condition a. But now, I'm confused because this can be re-interpreted as a || (b && c). Since parentheses have the highest precedence, does this mean that condition b is checked first? When using short-circuit operators I try to have the cheap computational test first, but I don't know whether that is a or b in this case. Eventually I'd probably guess that the parser sees token || token and still evaluates a first, but that's more thinking and guessing then I like to do :)
>>How should we fix the tests for increment and decrement? The test that results in an error for an invalid lvalue should be fixed, I think. The current precedence settings have ++ and -- highest of all except indexing. But what about ---a? That is an incompatibility with Matlab that will be pretty much impossible to fix. OTOH, it is probably not something that comes up too often.
Just scrap the test for ---a. It's such an obscure corner case and coders should use parentheses if they have any doubt about how the parser will be ordering operations.
>>Speaking of the left and right shift operators (<<, >>), should we remove them? Should we bother deprecating them, or just remove them? There is no actual use of these operators in any Octave scripting code, but users could have defined objects that use them. I don't see any in Octave Forge packages though, so maybe it would be OK to just dump them in 3.6.
What operators are these (<<, >>)? I just tried
in versions 3.0.5, 3.2.4, and 3.4.0. I think they have already been missing from Octave for quite awhile so they can probably just be dumped.
>>I'm attaching the new version of the patch. I'd like to get feedback, but please don't check it in just yet.
No, I'll let you check any patches in. I would also re-order the operator precedence chart to be from highest to lowest. It makes more sense for me to see item #1 be the most important. I would also add parentheses '()' as the first item to make it clear that they trump all other operators. Finally, the increment and decrement operators are listed as '+','-','++' whereas I think it should be only '++','--'.
|
Tue 19 Apr 2011 02:46:04 AM UTC, comment #12:
I'm attaching a new version of the patch. It makes the precedence levels the same as Matlab, except for the operators that Matlab does not have (OP=, ++/--, and <</>>).
I also fixed the test for exponentiation. Matlab docs specifically note that exponentiation is handled left to right. That's odd to me, but I don't see a compelling reason to do things differently.
I also changed the precedence levels of the || and &&, and | and & operators to match Matlab, with the OR operators having lower precedence than the AND operators. Should we try to warn about this change in 3.6?
How should we fix the tests for increment and decrement? The test that results in an error for an invalid lvalue should be fixed, I think. The current precedence settings have ++ and -- highest of all except indexing. But what about ---a? That is an incompatibility with Matlab that will be pretty much impossible to fix. OTOH, it is probably not something that comes up too often.
Speaking of the left and right shift operators (<<, >>), should we remove them? Should we bother deprecating them, or just remove them? There is no actual use of these operators in any Octave scripting code, but users could have defined objects that use them. I don't see any in Octave Forge packages though, so maybe it would be OK to just dump them in 3.6.
I'm attaching the new version of the patch. I'd like to get feedback, but please don't check it in just yet.
(file #23270)
|
Tue 19 Apr 2011 01:42:40 AM UTC, comment #11:
I think it is a bit risky, so I'd say put it on default. I would also like to take another look at it, so please let me do that and then I'll commit it.
|
Mon 18 Apr 2011 11:26:15 PM UTC, comment #10:
I was going to commit this patch for operator precedence. Should it go to 'stable', because this is a bug fix which users should probably see before 3.6. Or should it go to 'default', because messing with the parser might lead to instabilities and breaking of code that worked before?
|
Wed 06 Apr 2011 08:18:50 PM UTC, comment #9:
These are the results Matlab produces:
% Test 1
>> a = 2;
>> ~a++
??? ~a++
|
Error: Expression or statement is incomplete or incorrect.
(my comment: ++ is not defined in Matlab, so a++ does not work, too)
% Test 2
>> 2^3^5
ans =
32768
(my comment: mathematically wrong left to right evaluation)
% Test 3
a = 2;
---a
ans =
-2
(my comment: as -- is not defined in Matlab (the same as ++), Matlab's interpretation is: -(-(-a)))
|
Wed 06 Apr 2011 07:07:26 PM UTC, comment #8:
Could the original reporter verify the behavior of Matlab for the following tests?
|
Fri 18 Mar 2011 03:01:53 PM UTC, comment #7:
Yes, I'd say go ahead and add the tests.
My time available for working on Octave has been limited lately, but I'll try to take another look at fixing the precedence problems soon.
Thanks.
|
Wed 16 Mar 2011 05:15:49 PM UTC, comment #6:
John, should I add the operator precedence tests to Octave? They will currently fail for transpose, but I'd like to check them in so I can stop having to track them.
|
Sat 26 Feb 2011 12:53:07 AM UTC, comment #5:
I wrote a bunch of tests for operator precedence and have found 3 issues to address.
First, the following:
1) !a++ does not work with the new patch. Both '!' and '++' are at the same level of precedence, which usually implies left to right evaluation. However, !a is just a number which then can not be incremented by the '++' operator. The error message is
2) Exponents, like the assignment operators (=), are supposed to group right-to-left according to the documentation. This is not the case for either the old parser or the parser with the new patch. Instead exponents are evaluated left-to-right as any ordinary operator, but contrary to mathematical notions and the documentation.
3) Unary minus and the pre-decrement operator do not work together. This one is a real corner case and doesn't necessarily need to be fixed, but I thought I'd mention it.
|
Thu 24 Feb 2011 07:59:58 PM UTC, comment #4:
It's fine with me if the new tests are added to the existing test_parser.m file.
Thanks.
|
Thu 24 Feb 2011 06:55:35 PM UTC, comment #3:
It would be useful to have a set of tests which actually verify that operator precedence rules are respected. I don't mind writing those, but should they go in the existing test/test_parser.m or in a new file test/test_op_prec.m?
|
Thu 24 Feb 2011 07:45:09 AM UTC, comment #2:
The attached patch seems to fix the problem for me. All the tests still pass without parse errors, so it might be OK, but I have not done any more extensive testing so I'd like to hold off a bit before checking it in.
|
Mon 21 Feb 2011 03:41:20 AM UTC, comment #1:
Confirmed. Regardless of compatibility, there is a difference between the documented behavior and the actual behavior. According to the manual, section 8.8 on Operator Precedence, exponentiation has the highest priority of all operations and transpose is 2 levels lower. Thus, the exponentiation really should happen first followed by the transpose.
I'm going to lower the priority on the issue, however, since there is an obvious work-around of using parentheses to specify the exact order of operations.
|
Fri 18 Feb 2011 01:58:07 PM UTC, original submission:
Matlab evaluates the element-wise square and the transpose with equal importance and thus from left to right:
>> size(ones(1,3).^2.')
ans =
3 1
>> size(ones(1,3).'.^2)
ans =
3 1
Octave evaluates the transpose first and then the element-wise square and thus transposes the scalar exponent:
octave:1> size(ones(1,3).^2.')
ans =
1 3
octave:2> size(ones(1,3).'.^2)
ans =
3 1
I don't know which definition is superior, but I would opt for compatibility.
|