An arithmetic bug about addition (+) and multiplication (*)?

[Minjian]

I update my project from bsh-2.0b4.jar to bsh-3.0.jar, then some test scripts are failing.

I tested the following codes:

  int value1 = 1000 + 60 * 0;
  debugPrintln("debug value1 = " + value1);
  int value2 = 60 * 0 + 1000;
  debugPrintln("debug value2 = " + value2);
  int value3 = 1000 + (60 * 0);
  debugPrintln("debug value3 = " + value3);

Then I got this interesting output:

  debug value1 = 0
  debug value2 = 1000
  debug value3 = 1000

For value1, looks like it evaluates 1000 + 60 first and doesn't follow the correct arithmetic order for calculation. I fixed my test failures by using parenthesis like what I did for value3. Does anyone have similar issues in their project?

[opeongo]

Yes it looks like operator precedence was undone with commit 8e712bad002f33e5238dd00a0cb7c2ba2760322e. This commit needs to be reverted.

[opeongo]

Pull request #606 that fixes this problem

[nickl-]

Reverting commits is not fixing issues but discarding the fixes already made. Instead of looking for the commit that changed it, rather find the problem and solve it.

[opeongo]

In this case reverting the commit is the correct fix.

Looking at the javacc grammar it is obvious that the standard math operator precedence rules were removed by the original 8e712bad002f33e5238dd00a0cb7c2ba2760322e commit. In additional I added a set of test case that failed with the original commit, and succeeded after the revert.

The 8e712bad002f33e5238dd00a0cb7c2ba2760322e commit reduced the number of tokens for a supposed performance improvement, but the ordering of tokens is how operator precedence is expressed in javacc grammars. Reducing from 17 tokens down to 4 reduced the complexity all right, but that complexity was deliberate in order to implement standard math operator precedence. Before the commit that simplified the grammar

 1 + 2 * 3 = 7

After the simplification

1 + 2 * 3 = 9

I am absolutely certain I found the problem in what caused operator precedence to break, and I solved it with the revert. How else should I have approached this?

Also, why remove the test cases that I added? They show valid expressions and expected results. They should be left in, as with all tests, to validate behaviour and detect regressions.

[elbosso]

I second it - please revert this optimization from hell as it screws up virtually everything and makes the library virtually unusable! I am astonished that it takes this long a discussion - the commit breaks a central language feature and should have been reverted days after the issue was raised!

[elbosso]

I just saw that the pull request got merged half a year ago - how about closing this issue then?

[opeongo]

I just saw that the pull request got merged half a year ago - how about closing this issue then?

Hi @elbosso. My opinion is that the commit needs to be reverted again before the issue can be closed. However @nickl- is the one who made the original commit and who undid my reversion, and he seems to have a strong opinion on the matter. I will leave it up to him to weigh in on how this problem should be solved.

[nickl-]

I started work on the parser grammar to get this resolved but ran out of time. =(

Sure the commit is what broke operator precedence but there are also several other optimizations in the commit which we would benefit from keeping.

I will be resolving this momentarily. Thank you for your patience.

[opeongo]

there are also several other optimizations in the commit which we would benefit from keeping.

I doubt it.

The bulk of that commit is simply merging productions and losing all operator precedence. If there was a real optimization in there I do not see it. But anyway, proposed optimizations in this type of code should be backed up with benchmarks to show that there is real improvement. You can refactor the javacc code, but this will be translated into java and then in to byte code, and then jitted into machine code. Unless you are benchmarking execution timings in real world situations you will have no idea if your changes improve, decrease or do nothing to performance.

Best to just revert this and let it go. There are more significant bugs in this code that should be worked on instead.

“The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming.” Donald Knuth

also... "developers can spend a lot of time writing tightly optimized loops which gain small improvements in efficiency at the expense of huge reductions in maintainability."

[nickl-]

also... "developers can spend a lot of time writing tightly optimized loops which gain small improvements in efficiency at the expense of huge reductions in maintainability."

Yes that is what I mean... optimized complexity.

[opeongo]

Ok I submitted a PR (#671) for this problem that includes a test case.

[nickl-]

I thought this was fixed with #671 why does it remain open?