1372209 - Analysis to reject the comparison of variables with constants situated on the left side of the expression

Status: RESOLVED WORKSFORME

(Developer Infrastructure :: Source Code Analysis, enhancement, P5)

Description

[Andi [:andi]]

We shouldn't allow in our code comparisons where the constant is situated on the left side of the binary operation, like:

>>if (MAX_NUMBER > m_LocalNumber) {
>>...
>>}

this should be written as:

>>if (m_LocalNumber<MAX_NUMBER) {
>>...
>>}

Comment 1

[Sylvestre Ledru [:Sylvestre]]

This is just coding style/best practices, right?

Comment 2

[Andi [:andi]]

Yes, but this can be very easy detected/ corrected through static analysis.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

I don't think this is very high priority. It's going to require lots of changes to the codebase, and make working on our code more annoying, because even more technically correct code will fail only when run on try.

I'm inclined to not implement this, especially before we get a good story for running the analysis locally (such as bug 1328454), but I'll mark it as P5 for now.

Comment 4

[Alex Vincent [:WeirdAl]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #0)
> We shouldn't allow in our code comparisons where the constant is situated on
> the left side of the binary operation, like:
> 
> >>if (MAX_NUMBER > m_LocalNumber) {
> >>...
> >>}
> 
> this should be written as:
> 
> >>if (m_LocalNumber<MAX_NUMBER) {
> >>...
> >>}

Forgive me for being dense... but why does this matter?  What improvement does this bring?  I can't see it being a compiler issue:  compilers should be smart enough to reorder the arguments for the most optimal assembly code without much trouble, especially for primitives.

Comment 5

[Andi [:andi]]

(In reply to Alex Vincent [:WeirdAl] from comment #4)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #0)
> > We shouldn't allow in our code comparisons where the constant is situated on
> > the left side of the binary operation, like:
> > 
> > >>if (MAX_NUMBER > m_LocalNumber) {
> > >>...
> > >>}
> > 
> > this should be written as:
> > 
> > >>if (m_LocalNumber<MAX_NUMBER) {
> > >>...
> > >>}
> 
> Forgive me for being dense... but why does this matter?  What improvement
> does this bring?  I can't see it being a compiler issue:  compilers should
> be smart enough to reorder the arguments for the most optimal assembly code
> without much trouble, especially for primitives.

Performance wise, you are right, this checker should be more looked up as an enforcer for a good coding style.

Comment 6

[Andi [:andi]]

Giving this more thought I'm not sure we actually need this.