Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EC67] [Java] IncrementCheck irrelevant #60

Closed
arn-aud opened this issue Feb 23, 2023 · 3 comments
Closed

[EC67] [Java] IncrementCheck irrelevant #60

arn-aud opened this issue Feb 23, 2023 · 3 comments
Assignees
Labels
🗃️ rule rule improvment or rule development or bug java

Comments

@arn-aud
Copy link

arn-aud commented Feb 23, 2023

The IncrementCheck seems irrelevant since the java compiler produces the same byte code in many cases.

Steps to reproduce the behavior:

  1. write a class with a for-loop using i++ to increment the counter
  2. duplicate the class and replace i++ with ++i
  3. compile both classes
  4. compare the generated bytecode
  5. no differences found
@arn-aud arn-aud added the 💉 bug Something isn't working label Feb 23, 2023
@dedece35 dedece35 self-assigned this Apr 9, 2023
@dedece35
Copy link
Member

dedece35 commented Apr 9, 2023

Hi @arn-aud,

thank you for your issue.
after checking, this isn't so simple.

Here are my screenshots from our java test-project named ecoCode-java-test-project :

  • first and second screenshots : java sources
  • third screenshot : decompiled class files (from these java sources)

sources-1

sources-2

Capture d’écran 2023-04-09 à 22 51 51

What we can observe :

  • foo1 method : no difference
  • foo11 method : no difference
  • foo2 method : no difference
  • foo22 method : Here is a difference ==> indeed, compilation transformed ++counter to counter++, maybe because of no usage of counter variable after increment (contrary to the case of foo11 in which counter variable is returned)
  • foo3 method : Here is a difference ==> usage of +=operator instead of = operator with source variable
  • foo4 method : no difference
  • foo50 method (first NEW method to follow your recommended test) : no difference
  • foo51 method (second NEW method to follow your recommended test) : Here is a difference ==> ++i replaced with i++. For me, this is the same use case than foo22, because in this for loop, i variable is incremented in a last instruction and there is no direct use of it, like foo22

To conclude, I think, the optimization is smarter than we thought and for me, the rule seems to be relevant in some cases. But, as we can see, the default behaviour for compiler optimization is to put ++ operator as suffix. That's why, I agree with you that test use cases which there are no optimization by compiler are not so relevant.
We will discuss about it internally to see if e keep this rule or not. Furthermore, in RULES.md file there is no longer reference proving the usefullness of this rule (existed in 3rd version of CNUMr best practices book but no longer in 4th version)

@dedece35 dedece35 added the java label Aug 25, 2023
@dedece35 dedece35 changed the title IncrementCheck irrelevant [EC67] [Java] IncrementCheck irrelevant Aug 25, 2023
@dedece35
Copy link
Member

TO DISCUSS INSIDE CORE-TEAM : opinion ? to keep or not ? for me, to keep.

@glalloue
Copy link
Contributor

glalloue commented Oct 6, 2023

@dedece35 demonstration show that argument was not right bu do not show that the rule is efficient or not.
we can close this issue , but we need to keep the discussion to understand if the rule is right or not

(seen with @dedece35 and @jhertout )

@dedece35 dedece35 closed this as completed Oct 6, 2023
@dedece35 dedece35 added 🗃️ rule rule improvment or rule development or bug and removed 💉 bug Something isn't working __PRIO_MEDIUM__ 👀 👀 review done 👀 👀 review done - waiting for changes ⚖️⚖️ todiscuss_coreteam ⚖️⚖️ labels Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗃️ rule rule improvment or rule development or bug java
Projects
None yet
Development

No branches or pull requests

3 participants