Page MenuHomePhabricator

Clang-tidy readability: avoid const value return
Needs RevisionPublic

Authored by camillo on May 24 2017, 6:30 PM.

Details

Summary

Add a readability-const-value-return check to clang-tidy.

Sometimes people qualify return types with const. This was always unnecessarily verbose, but in modern C++ it has an even worse side effect: it prevents moving the returned object (unless one defines a move assignment operator that takes a "const &&", which is an even worse idea).

For example:

#include <iostream>

struct X {
  int a = 0;
  X& operator=(const X& other) {
    a = other.a;
    std::cout << "copied\n";
    return *this;
  }
  X& operator=(X&& other) {
    a = other.a;
    std::cout << "moved\n";
    return *this;
  }
};

const X f_bad() { return {}; }
X f_good() { return {}; }

int main() {
  X x;
  x = f_bad();
  x = f_good();
  return 0;
}

The assignment to x from f_bad forces a copy, while the assignment from f_good results in a move.

Therefore, it's better to avoid using const when returning things by value. This clang-tidy check warns about such situations.

The check ignores returns of const pointers (meaning * const, not const *); while pointless, these are not particularly harmful. It could be made laxer by ignoring all trivially copyable types (on the grounds that they won't have interesting move constructors/assigners anyway), or stricter by ignoring all const returns (on the grounds that, in the best case, it's just pointless verbosity). Or it could be made an option.

Diff Detail

Repository
rL LLVM

Event Timeline

camillo created this revision.May 24 2017, 6:30 PM
camillo updated this revision to Diff 100543.May 28 2017, 12:12 AM

Added fixit. Added more tests.

alexfh edited edge metadata.May 29 2017, 4:42 AM

The check ignores returns of const pointers (meaning * const, not const *); while pointless, these are not particularly harmful. It could be made laxer by ignoring all trivially copyable types (on the grounds that they won't have interesting move constructors/assigners anyway), or stricter by ignoring all const returns (on the grounds that, in the best case, it's just pointless verbosity). Or it could be made an option.

I'd prefer an option. It could be StrictMode, which is also supported as a global option, for example, by clang-tidy/misc/ArgumentCommentCheck.cpp.

alexfh added inline comments.May 29 2017, 4:57 AM
clang-tidy/readability/ConstValueReturnCheck.cpp
27

How about just matching definitions to avoid duplicate warnings?

test/clang-tidy/readability-const-value-return.cpp
1

Please add tests for:

  1. a function with multiple declarations and a definition
  2. function declarations/definitions in macros
  3. a class method
  4. implicit stuff (I'm not sure if a lambda can be made to return a const type without an explicit return type specification)
50

It would be nice to ensure the check doesn't trigger on template instantiation of this function as well.

I'm not opposed to this check, but I'm not keen on having a check that directly contradicts the output from another check. The cert-dcl21-cpp check will diagnose user's code when a postfix operator ++ or -- is *not* const-qualified. Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?

Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?

Currently there's no mechanism in clang-tidy to express dependencies or compatibility issues between checks. Moreover, we already have checks that are not meant to be run together, for example, readability-braces-around-statements and its google- incarnation (and other alias checks with different settings). That said, we could whitelist postfix increment and decrement operators in this check. Camillo, WDYT?

On a side note, the check's performance implications might be more important than the readability aspect of dropping the const, so the check might be a better fit for the performance category.

Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?

Currently there's no mechanism in clang-tidy to express dependencies or compatibility issues between checks. Moreover, we already have checks that are not meant to be run together, for example, readability-braces-around-statements and its google- incarnation (and other alias checks with different settings). That said, we could whitelist postfix increment and decrement operators in this check. Camillo, WDYT?

I can imagine a generic whitelist mechanism might be useful for this check. It could even be empty by default, but the documentation could call out cert-dcl21-cpp specifically and show an example of how you can run both checks.

On a side note, the check's performance implications might be more important than the readability aspect of dropping the const, so the check might be a better fit for the performance category.

I agree.

clang-tidy/readability/ConstValueReturnCheck.cpp
27

I'll echo this. I'd be worried about this triggering on 3rd party library headers that the user cannot control, otherwise.

97

No auto here, or elsewhere, when the type isn't explicitly spelled out in the initialization. Also, you can drop the local const qualifiers on value types.

Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?

Currently there's no mechanism in clang-tidy to express dependencies or compatibility issues between checks. Moreover, we already have checks that are not meant to be run together, for example, readability-braces-around-statements and its google- incarnation (and other alias checks with different settings). That said, we could whitelist postfix increment and decrement operators in this check. Camillo, WDYT?

I can imagine a generic whitelist mechanism might be useful for this check. It could even be empty by default, but the documentation could call out cert-dcl21-cpp specifically and show an example of how you can run both checks.

A generic whitelist of method/function names would make sense, if we had more use cases for it. It might also be quite tricky to implement: distinguishing between prefix and postfix increment/decrement operators would require specifying arguments, and allowing it for all types would need a support for pattern matching or optional omission of the type name on methods. All this seems to be an overkill so far. If we want this whitelisting be optional, we can add a boolean option specifically for these operators.

Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?

Currently there's no mechanism in clang-tidy to express dependencies or compatibility issues between checks. Moreover, we already have checks that are not meant to be run together, for example, readability-braces-around-statements and its google- incarnation (and other alias checks with different settings). That said, we could whitelist postfix increment and decrement operators in this check. Camillo, WDYT?

I can imagine a generic whitelist mechanism might be useful for this check. It could even be empty by default, but the documentation could call out cert-dcl21-cpp specifically and show an example of how you can run both checks.

A generic whitelist of method/function names would make sense, if we had more use cases for it. It might also be quite tricky to implement: distinguishing between prefix and postfix increment/decrement operators would require specifying arguments, and allowing it for all types would need a support for pattern matching or optional omission of the type name on methods. All this seems to be an overkill so far.

Good point on the prefix/postfix nature. This does seem like overkill.

If we want this whitelisting be optional, we can add a boolean option specifically for these operators.

In light of a more general solution, I say we don't add any configuration option. If it turns out people want to run both of these checks at the same time a lot in practice, we can address it with a more general mechanism to express dependencies/conflicts.

sbenza added a subscriber: sbenza.May 30 2017, 7:07 AM
sbenza added inline comments.
clang-tidy/readability/ConstValueReturnCheck.cpp
31

References are never const qualified, right?

test/clang-tidy/readability-const-value-return.cpp
51

We are missing one like:

template <typename T> T f();

where T is const X.

We also need to test for macros (and skip fixes there)

alexfh requested changes to this revision.Jul 11 2017, 6:40 AM
alexfh added a reviewer: sbenza.
This revision now requires changes to proceed.Jul 11 2017, 6:40 AM