Readability check for position of constant operand in binary operators

[EugeneZelenko]

Bugzilla Link	32513
Version	unspecified
OS	All

Extended Description

Will be good idea to have readability check for position of constant operand in binary operators.

For example:

if (nullptr == Pointer)

should be fixed as

if (Pointer == nullptr)

or

if (0 < Value)

to

if (Value > 0)

[EugeneZelenko]

See also #34799.

[EugeneZelenko]

It'll be reasonable to have option to disable this check for less and less or equal operators. In my work code base preferred syntax is similar to standard math notation, for example: (0 <= index) && (index < size).

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Assign the issue to you.
2. Fix the issue locally.
3. Run the test suite locally.
   3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can
   e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a git commit
5. Run git clang-format HEAD~1 to format your changes.
6. Submit the patch to Phabricator.
   6.1) Detailed instructions can be found here

For more instructions on how to submit a patch to LLVM, see our documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvm/issue-subscribers-good-first-issue

[Endilll]

@PiotrZSL can you provide some analysis and points of entry into codebase?

[PiotrZSL]

Create check readability-constant-operand-order or use other name.

1. match binary operator calls binaryOperator()
2. Add configuration option for types of operators, by default we want only == and !=, and other comparison operators should be configurable.
3. Add option to specify constant location (left or right, maybe introduce this as enum)
4. Based on configuration register matcher for left or right side.
5. Make sure to match as constant basically anything that is unevaluated expression (check other checks), reference to constexpr variable, literal, ...
6. write bunch of tests
7. write documentation, release notes
8. Implement "auto-fixes" that would just swap L with R, and invert operator.

To be honest this check is not so complicated

[11happy]

@PiotrZSL I am interested to work on this issue.

[PiotrZSL]

@11happy
Yes, but make it configurable as this is developer choose. Some projects may have this as a rule. Main purpose in past where that if you write code like this:

if (x == 0) {...}

developer could by mistake omit one =, and do an assignment by mistake, thats why some more strict (C) projects had a rule to put const value always on left side. At the same time some other projects prefer opposites, or do not set any rules.
Today issue with x = 0 is detectable indirectly with -Wparentheses.

Note that this check may not be high priority.
I would suggest you to search for something more performance or bugprone related.

[PiotrZSL]

Here is some info:

• https://www.moserware.com/2008/01/constants-on-left-are-better-but-this.html
• https://stackoverflow.com/questions/370366/why-put-the-constant-before-the-variable-in-a-comparison

Looks like this issue is also ... controversial.

@carlosgalvezp What do you think ?

[carlosgalvezp]

I believe this is quite subjective, but I think we can agree that the scope of this check is not to improve readability, but to avoid bugs. IMHO readability is hurt because it's reverses the order in which code is written (as other people in the sources above mentioned)

The sources also mention that this bug can be detected with -Wall, which I expect any serious project (at least one that goes the extra mile to enable clang-tidy) to already have enabled.

So I'm not sure this check would add that much extra value and I'd vote for not adding it. But that's just my opinion :)