Clang-tidy readability: avoid const value return

[PiotrZSL]

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.

[camillo]

Added fixit. Added more tests.

[alexfh]

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]

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.

[aaron.ballman]

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?

[alexfh]

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.

[aaron.ballman]

In D33531#767059, @alexfh wrote:

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.

[alexfh]

In D33531#767628, @aaron.ballman wrote:

In D33531#767059, @alexfh wrote:

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.

[aaron.ballman]

In D33531#767640, @alexfh wrote:

In D33531#767628, @aaron.ballman wrote:

In D33531#767059, @alexfh wrote:

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]

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)

[PiotrZSL]

Obsolete, this check is already added.

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
+  ConstValueReturnCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp

clang-tidy/readability/ConstValueReturnCheck.h

+//===--- ConstValueReturnCheck.h - clang-tidy--------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONST_VALUE_RETURN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONST_VALUE_RETURN_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Checks for functions that return a const-qualified value type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-const-value-return.html
+class ConstValueReturnCheck : public ClangTidyCheck {
+public:
+  ConstValueReturnCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONST_VALUE_RETURN_H

clang-tidy/readability/ConstValueReturnCheck.cpp

+//===--- ConstValueReturnCheck.cpp - clang-tidy----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ConstValueReturnCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void ConstValueReturnCheck::registerMatchers(MatchFinder *Finder) {
+  // Returning const pointers (not pointers to const) does not make much sense,
+  // returning const references even less so, but it is harmless and we don't
+  // bother checking it.
+  // Template parameter types may end up being pointers or references, so we
+  // skip those too.
+  Finder->addMatcher(functionDecl(returns(qualType(
+        isConstQualified(),
+        unless(anyOf(
+          isAnyPointer(),
+          references(type()),
+          templateTypeParmType(),
+          hasCanonicalType(templateTypeParmType())
+      ))))).bind("func"), this);
+}
+
+static bool isWithinRange(const Token& Tok, SourceRange Range,
+                          const SourceManager &Sources) {
+  return (Sources.isBeforeInTranslationUnit(
+              Range.getBegin(), Tok.getLocation()) &&
+          Sources.isBeforeInTranslationUnit(
+              Tok.getLocation(), Range.getEnd()));
+}
+
+// Looks for const tokens within SearchRange, ignoring those within
+// ReturnRange. If exactly one is found, it is returned.
+static llvm::Optional<Token> findConstToken(
+    SourceRange SearchRange, SourceRange ReturnRange,
+    const MatchFinder::MatchResult &Result) {
+  const SourceManager &Sources = *Result.SourceManager;
+  FileID FID;
+  unsigned Offset;
+  std::tie(FID, Offset) = Sources.getDecomposedLoc(SearchRange.getBegin());
+  StringRef FileData = Sources.getBufferData(FID);
+
+  Lexer RawLexer(Sources.getLocForStartOfFile(FID),
+                 Result.Context->getLangOpts(),
+                 FileData.begin(), FileData.begin() + Offset,
+                 FileData.end());
+
+  Token Tok;
+  llvm::Optional<Token> ConstTok;
+  int ConstFound = 0;
+  while (!RawLexer.LexFromRawLexer(Tok)) {
+    // Stop at the end of SearchRange.
+    if (Sources.isBeforeInTranslationUnit(
+        SearchRange.getEnd(), Tok.getLocation()))
+      break;
+    if (Tok.is(tok::raw_identifier)) {
+      IdentifierInfo &Info = Result.Context->Idents.get(StringRef(
+          Sources.getCharacterData(Tok.getLocation()), Tok.getLength()));
+      Tok.setIdentifierInfo(&Info);
+      Tok.setKind(Info.getTokenID());
+    }
+    // Ignore consts inside ReturnRange.
+    if (isWithinRange(Tok, ReturnRange, Sources))
+      continue;
+    if (Tok.is(tok::kw_const)) {
+      ++ConstFound;
+      ConstTok = Tok;
+    }
+  }
+
+  if (ConstFound > 1)
+    ConstTok.reset();
+
+  return ConstTok;
+}
+
+void ConstValueReturnCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func");
+  auto Diag = diag(Func->getLocation(),
+                   "function %0 has a const value return type; "
+                   "this can cause unnecessary copies")
+      << Func;
+
+  auto ReturnType = Func->getReturnType();
+  if (!ReturnType.isLocalConstQualified())
+    return;
+
+  const auto FuncRange = Func->getSourceRange();
+  if (FuncRange.isInvalid())
+    return;
+
+  // When using trailing return type syntax, an invalid range is returned,
+  // so we cannot generate the fixit.
+  const auto ReturnRange = Func->getReturnTypeSourceRange();
+  if (ReturnRange.isInvalid())
+    return;
+
+  // getReturnTypeSourceRange does not include the qualifiers, so we have to
+  // find the "const" ourselves.
+  auto NameLoc = Func->getNameInfo().getLoc();
+  auto MaybeConst = findConstToken({FuncRange.getBegin(), NameLoc},
+                                   ReturnRange, Result);
+  if (!MaybeConst)
+    return;
+
+  auto TokLoc = MaybeConst->getLocation();
+  Diag << FixItHint::CreateRemoval(
+      CharSourceRange::getTokenRange(TokLoc, TokLoc));
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
+#include "ConstValueReturnCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"

 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
+    CheckFactories.registerCheck<ConstValueReturnCheck>(
+        "readability-const-value-return");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<DeleteNullPointerCheck>(
         "readability-delete-null-pointer");
     CheckFactories.registerCheck<DeletedDefaultCheck>(
         "readability-deleted-default");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");

docs/clang-tidy/checks/list.rst

.. toctree::
    performance-implicit-cast-in-loop
    performance-inefficient-string-concatenation
    performance-inefficient-vector-operation
    performance-type-promotion-in-math-fn
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
+   readability-const-value-return
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name

docs/clang-tidy/checks/readability-const-value-return.rst

+.. title:: clang-tidy - readability-const-value-return
+
+readability-const-value-return
+==============================
+
+Checks for functions that return a ``const``-qualified value type. That use of
+``const`` is superfluous, and prevents moving the object. Therefore, it should
+be avoided.
+
+Examples:
+
+.. code-block:: c++
+
+  const Foo f_bad();
+  Foo f_good();
+
+  Foo foo;
+  foo = f_bad();   // This requires a copy.
+  foo = f_good();  // This can use a move.
+
+
+Note that this does not apply to returning pointers or references to const
+objects:
+
+.. code-block:: c++
+
+  const Foo* f_ptr();  // No issue.
+  const Foo& f_ref();  // No issue.

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

+// RUN: %check_clang_tidy %s readability-const-value-return %t
+
+namespace std {
+template<class> class function;
+template<class R, class... ArgTypes> class function<R(ArgTypes...)>;
+}
+
+struct X {
+  int a;
+};
+
+// These should trigger the check.
+const X f_returns_const_value();
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: function 'f_returns_const_value' has a const value return type; this can cause unnecessary copies [readability-const-value-return]
+// CHECK-FIXES: {{^}}X f_returns_const_value();{{$}}
+
+X const f_returns_const_value2();
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: function 'f_returns_const_value2' has a const value return type; this can cause unnecessary copies [readability-const-value-return]
+// CHECK-FIXES: {{^}}X f_returns_const_value2();{{$}}
+
+// When using trailing return type syntax, we cannot generate the fixit, because we cannot get the return type range.
+auto f_returns_const_value3() -> const X;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f_returns_const_value3' has a const value return type; this can cause unnecessary copies [readability-const-value-return]
+
+auto f_returns_const_value4() -> X const;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f_returns_const_value4' has a const value return type; this can cause unnecessary copies [readability-const-value-return]
+
+// Make sure we get the right "const".
+const std::function<int(const char *)> f_returns_const_fun();
+// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: function 'f_returns_const_fun' has a const value return type; this can cause unnecessary copies [readability-const-value-return]
+// CHECK-FIXES: {{^}}std::function<int(const char *)> f_returns_const_fun();{{$}}
+
+std::function<int(const char *)> const f_returns_const_fun2();
+// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: function 'f_returns_const_fun2' has a const value return type; this can cause unnecessary copies [readability-const-value-return]
+// CHECK-FIXES: {{^}}std::function<int(const char *)> f_returns_const_fun2();{{$}}
+
+// These should not trigger the check.
+X f_returns_nonconst_value();
+const X* f_returns_pointer_to_const();
+X* const f_returns_const_pointer();
+const X& f_returns_reference_to_const();
+
+typedef X* Xptr;
+typedef X& Xref;
+const Xptr f_returns_typedef_pointer();
+const Xref f_returns_typedef_reference();
+
+// A template parameter type may end up being a pointer or a reference, so we skip it too.
+template <typename T>
+const T f_returns_template_param();
+
+template <typename T>
+struct S {
+  using Q = T;
+  const Q f_returns_template_param_alias();
+};
+
+using R = X&;
+const R f_returns_reference_alias();