A8-4-7: Regression with reference types

[jsinglet]

Affected rules

• A8-4-7

Description

A false positive has been reported for the example below.

Additionally:

• the definition of "trivial to copy" is not encoded; https://eel.is/c++draft/basic.types.general#9.sentence-2
• The computation of size is wrong in certain instances.

Example

class A8_4_7 {
 public:
  std::array<char, 20UL> values;
};
void output(const A8_4_7 &a847) noexcept { // trivial to copy but size is larger than 2 words. (false positive)
  std::cout << a847.values[0] << std::endl;
}```

[rcseacord]

For background, this AUTOSAR rule seems poorly conceived. The rule is strictly a performance rule. The rule states that "passing an argument by value documents that the argument won't be modified" but unlike passing by reference to cost eliminates indirection in the function body. From a safety perspective, there is no real advantage in passing by value over passing by reference to const. I'm not sure enforcing this rule adds much if any value, and of course, changing code to comply with a rule always costs efforts and may introduce additional defects.

[rcseacord]

The enforcement guidance on which this rule is based "F.16: For “in” parameters, pass cheaply-copied types by value and others by reference to const" says:

• (Simple) ((Foundation)) Warn when a parameter being passed by value has a size greater than 2 * sizeof(void*). Suggest using a reference to const instead.
• (Simple) ((Foundation)) Warn when a parameter passed by reference to const has a size less than 2 * sizeof(void*). Suggest passing by value instead.
• (Simple) ((Foundation)) Warn when a parameter passed by reference to const is moved.

This is fairly different enforcement from what we currently have.

[rcseacord]

In the example, while the array is trivially copyable it is likely larger than 2 * sizeof(void*) which is an implementation-defined value so this clearly seems to be a false positive. A larger question is if this required rule should be enforced at all.

[jsinglet]

Agreed @rcseacord. Testing locally the computed size is 20 bytes and the word size is 8 bytes. So since it is longer than 2 words it is not eligible to be passed by value (under this rule) so it is clearly a false positive, trivially copyable or not.

Some more notes on what needs to be fixed:

• Needs to factor in definition of trivial to copy / identify the cases where that makes a difference.
• The calculation of size needs a call to v.getType().stripType().getSize() to ensure that the correct size is begin calculated.

Example:
Calling getType().getSize() on const A8_4_7 &a847 will yield 8, when what is expected is 20.

• Reference types are explicitly excluded in the query -- There is an edge case that was introduced (the catch block clause) but the exclusion of all reference types is too broad since some will need to be flagged. For example:

struct A { std::uint32_t a; };

void f1(const A &a){} // this should be flagged 

[knewbury01]

this might have been solved in this PR , will check