[clang-tidy] Added option for disabling const qualifiers in readability-qualified-auto

[njames93]

Summary

Adds an option called AddConstToQualified to readability-qualified-auto to toggle adding const to the auto typed pointers and references. By default its enabled but in the LLVM module its disabled.

[lebedev.ri]

SGTM, but please wait for the reviewers.

[njames93]

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp

209

If a variables is typed as auto x = cast<const int*>(y), should the behaviour be const auto*x = ... even if AddConstQualifier is turned off. Thereby making the AddConstQualifier check only there to ensure that variables already typed as auto * and auto & get const qualifier if necessary

[merge_guards_bot]

Unit tests: pass. 62260 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[• Quuxplusone]

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst

50

Is it worth adding an example of a double pointer?

auto BarN = cast<int **>(FooN);

Does that become auto* or auto** (and why)? My wild guess is that it becomes auto* (and because nobody cares about double pointers), but I could be wrong.

73

s/Will be/will be/
s/will get be/will be/

In the preceding section you give an example with volatile. How about here? What happens with

auto *volatile Foo3 = cast<const int *>(Bar3);
auto *Foo4 = cast<volatile int *>(Bar4);

Do they become

const auto *volatile Foo3 = cast<const int *>(Bar3);
volatile auto *Foo4 = cast<volatile int *>(Bar4);

as I would expect?

78

How does this option interact with

const int range[10];
for (auto&& elt : range)

Does it (incorrectly IMHO) make that const auto&& elt, or (correctly IMHO) const auto& elt, or (also correctly) leave it as [auto&&, which Always Works](https://quuxplusone.github.io/blog/2018/12/15/autorefref-always-works/)?

clang-tools-extra/test/clang-tidy/checkers/llvm-qualified-auto.cpp

21

It is worth adding one test case to show that the LLVM alias does not gratuitously remove const from declarations:

auto const &ExtendedConstPtr = getCIntPtr();
// becomes
auto *const &ExtendedConstPtr = getCIntPtr();

[njames93]

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst

50

Double pointers are just resolved to auto *.

73

Good spot on the Will/be/get/(whatever I tried to type)

the check keeps local volatile qualifiers, but it wont add volatile pointer (or ref) qualifiers even if the deduced type is a volatile pointer. In the first patch we came to the conclusion not to add them.

78

RValueReferences are ignored

clang-tools-extra/test/clang-tidy/checkers/llvm-qualified-auto.cpp

21

That test case looks malformed as the function getCIntPtr() doesn't return a reference. as for removing const etc those cases are all in the readability-qualified-auto.cpp test file

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst

50

They resolve to auto * today but I could see a real argument that they should resolve to auto ** instead based on the same logic of: don't make the reader infer pointer/references and qualifiers.

[njames93]

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst

50

One change per patch...

[njames93]

• Always add const to auto typed variable.

This update adds const to variables just typed with auto, but wont enforce checking on auto * or auto & unless AddConstToQualified is set.

[merge_guards_bot]

Unit tests: fail. 62275 tests passed, 1 failed and 827 were skipped.

failed: Clang.CodeGenOpenCL/amdgpu-features.cl

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst

50

I wasn't suggesting a change to this patch, I was checking to see if there's sentiment that we missed this case in the first place. Sorry for not making that more clear.

[njames93]

• Streamline fixits
• Add documentation about double pointers, maybe a follow up patch to fix

[merge_guards_bot]

Unit tests: fail. 62275 tests passed, 1 failed and 827 were skipped.

failed: Clang.CodeGenOpenCL/amdgpu-features.cl

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[njames93]

• Small reformat

[merge_guards_bot]

Unit tests: pass. 62383 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp

276

Not specific to this patch, so feel free to address in another one -- I think you should be passing Var instead of Var->getName() -- the diagnostics engine does magic to format the name properly for any NamedDecl, including proper quoting. (Similar applies elsewhere.)

[njames93]

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp

276

I couldn't do that as it actually messes the quotes up: 'auto &'TdNakedCRef''

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp

276

Ah, good point. I was thinking about variable template specializations, but I'm not certain there's an actual issue with them -- it seems like we really want just the base identifier here.

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<PreferIsaOrDynCastInConditionalsCheck>(
         "llvm-prefer-isa-or-dyn-cast-in-conditionals");
     CheckFactories.registerCheck<PreferRegisterOverUnsignedCheck>(
         "llvm-prefer-register-over-unsigned");
     CheckFactories.registerCheck<readability::QualifiedAutoCheck>(
         "llvm-qualified-auto");
     CheckFactories.registerCheck<TwineLocalCheck>("llvm-twine-local");
   }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    Options.CheckOptions["llvm-qualified-auto.AddConstToQualified"] = "0";
+    return Options;
+  }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<LLVMModule> X("llvm-module",
                                                   "Adds LLVM lint checks.");
 
 } // namespace llvm_check
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the LLVMModule.
 volatile int LLVMModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.h

 /// 'auto*' or 'const auto *' and reference variables declared as:
 /// 'const auto &'.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-qualified-auto.html
 class QualifiedAutoCheck : public ClangTidyCheck {
 public:
   QualifiedAutoCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : ClangTidyCheck(Name, Context),
+        AddConstToQualified(Options.get("AddConstToQualified", true)) {}
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool AddConstToQualified;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_QUALIFIEDAUTOCHECK_H

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp

bool isAutoPointerConst(QualType QType) {
   QualType Pointee =
       cast<AutoType>(QType->getPointeeType().getTypePtr())->desugar();
   assert(!Pointee.isNull() && "can't have a null Pointee");
   return Pointee.isConstQualified();
 }
 
 } // namespace
 
+void QualifiedAutoCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AddConstToQualified", AddConstToQualified);
+}
+
 void QualifiedAutoCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
     return; // Auto deduction not used in 'C or C++03 and earlier', so don't
             // register Matchers.
   auto ExplicitSingleVarDecl =
       [](const ast_matchers::internal::Matcher<VarDecl> &InnerMatcher,
          llvm::StringRef ID) {
         return declStmt(
Finder->addMatcher(
                     pointee(hasUnqualifiedType(qualType().bind("type")),
                             unless(functionType())))))),
                 anyOf(hasAncestor(
                           functionDecl(hasAnyTemplateArgument(IsBoundToType))),
                       hasAncestor(classTemplateSpecializationDecl(
                           hasAnyTemplateArgument(IsBoundToType))))),
           "auto"),
       this);
+  if (!AddConstToQualified)
+    return;
   Finder->addMatcher(ExplicitSingleVarDecl(
                          hasType(pointerType(pointee(autoType()))), "auto_ptr"),
                      this);
   Finder->addMatcher(
       ExplicitSingleVarDecl(hasType(lValueReferenceType(pointee(autoType()))),
                             "auto_ref"),
       this);
 }
if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("auto")) {
     SourceLocation FixitLoc = FixItRange.getBegin();
     for (SourceRange &Range : RemoveQualifiersRange) {
       if (Range.getBegin() < FixitLoc)
         FixitLoc = Range.getBegin();
     }
 
     std::string ReplStr = [&] {
       llvm::StringRef PtrConst = isPointerConst(Var->getType()) ? "const " : "";
       llvm::StringRef LocalConst = IsLocalConst ? "const " : "";
       llvm::StringRef LocalVol = IsLocalVolatile ? "volatile " : "";
       llvm::StringRef LocalRestrict = IsLocalRestrict ? "__restrict " : "";
       return (PtrConst + "auto *" + LocalConst + LocalVol + LocalRestrict)
           .str();
     }();
 
     DiagnosticBuilder Diag =
         diag(FixitLoc, "'%0%1%2auto %3' can be declared as '%4%3'")
         << (IsLocalConst ? "const " : "")
         << (IsLocalVolatile ? "volatile " : "")
         << (IsLocalRestrict ? "__restrict " : "") << Var->getName() << ReplStr;
 
     for (SourceRange &Range : RemoveQualifiersRange) {
       Diag << FixItHint::CreateRemoval(
-          CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()));
+          CharSourceRange::getCharRange(Range));
     }
 
     Diag << FixItHint::CreateReplacement(FixItRange, ReplStr);
     return;
   }
   if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("auto_ptr")) {
     if (!isPointerConst(Var->getType()))
       return; // Pointer isn't const, no need to add const qualifier.
if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("auto_ptr")) {
     }
     if (Var->getType().isLocalRestrictQualified()) {
       llvm::Optional<Token> Token =
           findQualToken(Var, Qualifier::Restrict, Result);
       if (!Token || Token->getLocation().isMacroID())
         return;
     }
 
-    CharSourceRange FixItRange;
+        SourceLocation InsertPos;
     if (llvm::Optional<SourceRange> TypeSpec =
             getTypeSpecifierLocation(Var, Result)) {
-      FixItRange = CharSourceRange::getCharRange(*TypeSpec);
-      if (FixItRange.isInvalid())
+      InsertPos = TypeSpec->getBegin();
+      if (TypeSpec->isInvalid() || TypeSpec->getBegin().isMacroID() ||
+          TypeSpec->getEnd().isMacroID())
         return;
     } else
       return;
 
     DiagnosticBuilder Diag =
-        diag(FixItRange.getBegin(),
+        diag(InsertPos,
              "'auto *%0%1%2' can be declared as 'const auto *%0%1%2'")
         << (Var->getType().isLocalConstQualified() ? "const " : "")
         << (Var->getType().isLocalVolatileQualified() ? "volatile " : "")
         << Var->getName();
-    Diag << FixItHint::CreateReplacement(FixItRange, "const auto *");
+    Diag << FixItHint::CreateInsertion(InsertPos, "const ");
     return;
   }
   if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("auto_ref")) {
     if (!isPointerConst(Var->getType()))
       return; // Pointer isn't const, no need to add const qualifier.
     if (!isAutoPointerConst(Var->getType()))
       // Const isnt wrapped in the auto type, so must be declared explicitly.
       return;
 
-    CharSourceRange FixItRange;
+    SourceLocation InsertPos;
     if (llvm::Optional<SourceRange> TypeSpec =
             getTypeSpecifierLocation(Var, Result)) {
-      FixItRange = CharSourceRange::getCharRange(*TypeSpec);
-      if (FixItRange.isInvalid())
+      InsertPos = TypeSpec->getBegin();
+      if (TypeSpec->isInvalid() || TypeSpec->getBegin().isMacroID() ||
+          TypeSpec->getEnd().isMacroID())
         return;
     } else
       return;
 
     DiagnosticBuilder Diag =
-        diag(FixItRange.getBegin(),
+        diag(InsertPos,
              "'auto &%0' can be declared as 'const auto &%0'")
         << Var->getName();
-    Diag << FixItHint::CreateReplacement(FixItRange, "const auto &");
+    Diag << FixItHint::CreateInsertion(InsertPos, "const ");
     return;
   }
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 - New alias :doc:`cert-dcl51-cpp
   <clang-tidy/checks/cert-dcl51-cpp>` to
   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`readability-qualified-auto
+  <clang-tidy/checks/readability-qualified-about>` check now supports a 
+  `AddConstToQualified` to enable adding ``const`` qualifiers to variables
+  typed with ``auto *`` and ``auto &``.
+
 - Improved :doc:`readability-redundant-string-init
   <clang-tidy/checks/readability-redundant-string-init>` check now supports a
   `StringNames` option enabling its application to custom string classes. The 
   check now detects in class initializers and constructor initializers which 
   are deemed to be redundant.
 
 Renamed checks
 ^^^^^^^^^^^^^^

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst

 .. title:: clang-tidy - readability-qualified-auto
 
 readability-qualified-auto
 ==========================
 
-Adds pointer and ``const`` qualifications to ``auto``-typed variables that are deduced
-to pointers and ``const`` pointers.
+Adds pointer qualifications to ``auto``-typed variables that are deduced to 
+pointers.
 
-`LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html>`_ advises to
-make it obvious if a ``auto`` typed variable is a pointer, constant pointer or 
-constant reference. This check will transform ``auto`` to ``auto *`` when the 
-type is deduced to be a pointer, as well as adding ``const`` when applicable to
-``auto`` pointers or references
+`LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto>`_
+advises to make it obvious if a ``auto`` typed variable is a pointer. This 
+check will transform ``auto`` to ``auto *`` when the type is deduced to be a
+pointer.
 
 .. code-block:: c++
 
-  for (auto &Data : MutatableContainer) {
-    change(Data);
-  }
-  for (auto &Data : ConstantContainer) {
-    observe(Data);
-  }
   for (auto Data : MutatablePtrContainer) {
     change(*Data);
   }
   for (auto Data : ConstantPtrContainer) {
     observe(*Data);
   }
 
 Would be transformed into:
 
 .. code-block:: c++
 
-  for (auto &Data : MutatableContainer) {
-    change(Data);
-  }
-  for (const auto &Data : ConstantContainer) {
-    observe(Data);
-  }
   for (auto *Data : MutatablePtrContainer) {
     change(*Data);
   }
   for (const auto *Data : ConstantPtrContainer) {
     observe(*Data);
   }
 
-Note const volatile qualified types will retain their const and volatile qualifiers.
+Note ``const`` ``volatile`` qualified types will retain their ``const`` and 
+``volatile`` qualifiers. Pointers to pointers will not be fully qualified.
 
 .. code-block:: c++
 
    const auto Foo = cast<int *>(Baz1);
    const auto Bar = cast<const int *>(Baz2);
-  volatile auto FooBar = cast<int*>(Baz3);
+   volatile auto FooBar = cast<int *>(Baz3);
+   auto BarFoo = cast<int **>(Baz4);
 
 Would be transformed into:
 
 .. code-block:: c++
 
    auto *const Foo = cast<int *>(Baz1);
    const auto *const Bar = cast<const int *>(Baz2);
-  auto *volatile FooBar = cast<int*>(Baz3);
+   auto *volatile FooBar = cast<int *>(Baz3);
+   auto *BarFoo = cast<int **>(Baz4);
+
+Options
+-------
+
+.. option:: AddConstToQualified
+   
+   When set to `1` the check will add const qualifiers variables defined as
+   ``auto *`` or ``auto &`` when applicable.
+   Default value is '1'.
+
+.. code-block:: c++
+
+   auto Foo1 = cast<const int *>(Bar1);
+   auto *Foo2 = cast<const int *>(Bar2);
+   auto &Foo3 = cast<const int &>(Bar3);
+
+   If AddConstToQualified is set to `0`,  it will be transformed into:
+
+.. code-block:: c++
+
+   const auto *Foo1 = cast<const int *>(Bar1);
+   auto *Foo2 = cast<const int *>(Bar2);
+   auto &Foo3 = cast<const int &>(Bar3);
+
+   Otherwise it will be transformed into:
+
+.. code-block:: c++
+
+   const auto *Foo1 = cast<const int *>(Bar1);
+   const auto *Foo2 = cast<const int *>(Bar2);
+   const auto &Foo3 = cast<const int &>(Bar3);
 
-This check helps to enforce this `LLVM Coding Standards recommendation
-<https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto>`_.
+   Note in the LLVM alias, the default value is `0`.

clang-tools-extra/test/clang-tidy/checkers/llvm-qualified-auto.cpp

+// RUN: %check_clang_tidy %s llvm-qualified-auto %t
+
+// This check just ensures by default the llvm alias doesn't add const
+// qualifiers to decls, so no need to copy the entire test file from
+// readability-qualified-auto.
+
+int *getIntPtr();
+const int *getCIntPtr();
+
+void foo() {
+  auto NakedPtr = getIntPtr();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto NakedPtr' can be declared as 'auto *NakedPtr'
+  // CHECK-FIXES: {{^}}  auto *NakedPtr = getIntPtr();
+  auto NakedConstPtr = getCIntPtr();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'auto NakedConstPtr' can be declared as 'const auto *NakedConstPtr'
+  // CHECK-FIXES: {{^}}  const auto *NakedConstPtr = getCIntPtr();
+  auto *Ptr = getIntPtr();
+  auto *ConstPtr = getCIntPtr();
+  auto &NakedRef = *getIntPtr();
+  auto &NakedConstRef = *getCIntPtr();
+}