codeql-coding-standards icon indicating copy to clipboard operation
codeql-coding-standards copied to clipboard

`A5-2-6`: Exclude cases with the same binary operator

Open lcartey opened this issue 1 year ago • 3 comments

Affected rules

  • A5-2-6

Description

~~It's not well defined by the standard, but I think it's reasonable to exclude -> and . when considering whether an operand of a logical operator is binary.~~

Update: We thought the query was incorrectly reporting -> and . as binary operators. However, the problem was a confusion over this example:

foo->bar() && foo->baz() && foo->bang()

Where the query is reporting that this should be bracketed like so:

(foo->bar() && foo->baz()) && foo->bang()

This is a grey area in the rule itself - technically the title implies we should report this case. However, the rationale states:

Parentheses are required to add clarity in logical expressions making code easier to review versus code based only C++ operator precedence rules.

Which I think argues against asking for the developers to add unnecessary brackets in this case.

Proposed next steps:

  • Exclude cases where the nested binary operator is the same as the parent binary operator.
  • Refine the wording of the alert to ensure that it's clear where the brackets should be applied.

Example

foo->bar() && foo->baz() && foo->bang()

lcartey avatar Mar 03 '23 12:03 lcartey

Reviewing the query, the description above is not accurate. The query already only reports BinaryOperations, which does not include FieldAccesses (such as -> and .). I have investigated whether these could be caused by the use of operator->, either independently or in conjunction with templates but cannot reproduce the problem.

lcartey avatar Apr 19 '23 22:04 lcartey

Issue body updated: the problem has been identified as related to (a) confusion caused by the alert message about where the brackets should apply and (b) an over-zealous rule.

lcartey avatar May 26 '23 11:05 lcartey

I would like to report a similar over-zealous case when using the same comparison operator:

(a == 0) && (b == 0) // True negative, OK

(a == 0) && (b == 0) && ( c == 0) // Triggers A5-2-6 warning (zealous)

((a == 0) && (b == 0)) && ( c == 0) // Needs to do this to avoid A5-2-6 warning, which alters readability of code

AUTOSAR C++14 is not really clear for this specific case.

nbusser avatar Jun 02 '24 03:06 nbusser