boost-lexical-cast-reduntant-type

[Prazek]

Summary

in progress

[alexfh]

clang-tidy/boost/LexicalCastRedundantTypeCheck.cpp

48

Now I start understanding. It looks like BuiltinTypeLoc::getLocalSourceRange() always returns a range of a single token, which doesn't work for long long, unsigned int and the like. I'm not sure whether it's reasonable to keep the information about the last token of the builtin type name in BuiltinTypeLoc, but it's possible to use lexer to work around this by looking at the tokens after the first one (see the getTokenAtLoc() function in clang-tidy/google/IntegerTypesCheck.cpp, for example).

clang-tidy/boost/LexicalCastRedundantTypeCheck.h

19

Why is the check specific to boost::lexical_cast<>? The same transformation might be equally useful for C++ named casts (like static_cast<>), a C-style cast or any other expression literally mentioning its resulting type name (e.g. llvm::cast<>, llvm::dyn_cast<>, etc.).

[Prazek]

clang-tidy/boost/LexicalCastRedundantTypeCheck.cpp

48

Ok, I will take a look at this, but I find it very unintuive - I think that this function should also work for PODs

clang-tidy/boost/LexicalCastRedundantTypeCheck.h

19

That's right. I was planning to ship this first, because the code base which I will be testing this consists of much more lexical_cast.
Next step would be to generalize it for other casts. I am not sure how it should be, but I supose that it should not be one check doing everyhing.
I would prefer to add:

• boost-lexical-cast-reduntant-type
• llvm-casts-redundant-type
• modernize-casts-redundant-type

And the last one would do all the job, and it would be configurable so other checks can add their casts.

[alexfh]

clang-tidy/boost/LexicalCastRedundantTypeCheck.h

19

I don't think there's a significant difference in how boost::lexical_cast and llvm::(dyn_)?cast(_or_null)? should be handled. The difference is mainly in the list of function/type names. A slightly (though not too much) different case is when the type isn't completely repeated in the template parameter (e.g. const VarDecl *Matched = dyn_cast<VarDecl>(*D->decl_begin()); => const auto *Matched = dyn_cast<VarDecl>(*D->decl_begin());), but it can also be handled by the same check. Ultimately, I don't see a reason for three new checks here. I somehow like the idea to extend the already existing modernize-use-auto check instead of adding a new one.

What do you think?

[Prazek]

clang-tidy/boost/LexicalCastRedundantTypeCheck.h

19

That's right, but what I see from other checks, is that some of them redirects to other.
I think it would be nice to specify -checks=boost* and have check that would look for lexical_casts.
I have in plans to add at least one more boost check (changing lexical_cast<std::string> -> std::to_string).

The other thing is that I don't think that having modernize-use-auto looking for boost::lexical_cast is good idea.

[alexfh]

clang-tidy/boost/LexicalCastRedundantTypeCheck.h

19

I think it would be nice to specify -checks=boost* and have check that would look for lexical_casts.

A check can be registered under multiple names specifically for this purpose.

I don't think that having modernize-use-auto looking for boost::lexical_cast is good idea.

Why not? What undesirable effects can this result in?

In any case, we should make the list of functions handled by modernize-use-auto configurable so we can specify a custom list of types when registering the check in a project-specific module.

[Eugene.Zelenko]

I think will be good idea to extend modernize-use-auto instead of introducing new checks, especially multiple ones. See also PR25499.

[Prazek]

clang-tidy/boost/LexicalCastRedundantTypeCheck.h

19

Hmm, ok, I think you are right. But I am not sure is it easy to do something like this:

1. extend modernize-use-auto
2. add boost-redundant-lexical-cast as link to modernize-use-auto, but only that would look for lexical_cast (and not iterators, new, or other things)

I guess I would have to add some options that would switch off some things in modernize-use-auto.
And I am not sure about default functions that we should match - either STL things like static_cast, dynamic_cast, reinterpret_cast
or everything with boost and llvm.

[Prazek]

clang-tidy/boost/LexicalCastRedundantTypeCheck.cpp

48

Ok, so what is the plan - should I use the same hacks as in IntegerTypesCheck, or can I fix getLocalSourceRange to give full type?

[alexfh]

clang-tidy/boost/LexicalCastRedundantTypeCheck.cpp

48

As Richard Smith suggested, fixing getLocalSourceRange might make sense. He'd probably be the best reviewer for the change implementing that.

[alexfh]

clang-tidy/boost/LexicalCastRedundantTypeCheck.h

19

I guess I would have to add some options that would switch off some things in modernize-use-auto.
And I am not sure about default functions that we should match - either STL things like static_cast,
dynamic_cast, reinterpret_cast or everything with boost and llvm.

I think, the default one should look for the standard casts (note, they have nothing to do with STL, they are language constructs) and have an option with a list of custom cast-like functions. The aliases (say, boost-use-auto) could handle all standard cases and add boost::lexical_cast to the aforementioned list of custom cast-like functions.

[Prazek]

modernize-use-auto can handle it right now

clang-tidy/CMakeLists.txt

add_clang_library(clangTidy
   clangSema
   clangStaticAnalyzerCore
   clangStaticAnalyzerFrontend
   clangTooling
   clangToolingCore
   )
 
 add_subdirectory(tool)
+add_subdirectory(boost)
 add_subdirectory(cert)
 add_subdirectory(llvm)
 add_subdirectory(cppcoreguidelines)
 add_subdirectory(google)
 add_subdirectory(misc)
 add_subdirectory(modernize)
 add_subdirectory(performance)
 add_subdirectory(readability)
 add_subdirectory(utils)

clang-tidy/boost/BoostTidyModule.cpp

+//===------- BoostTidyModule.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 "LexicalCastRedundantTypeCheck.h"
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace boost {
+
+class ModernizeModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<LexicalCastRedundantTypeCheck>(
+        "boost-lexical-cast-redundant-type");
+  }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    return Options;
+  }
+};
+
+// Register the ModernizeTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<ModernizeModule> X("boost-module",
+                                                       "Add boost checks.");
+
+} // namespace boost
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the BoostModule.
+volatile int BoostModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tidy/boost/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyBoostModule
+  BoostTidyModule.cpp
+  LexicalCastRedundantTypeCheck.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )

clang-tidy/boost/LexicalCastRedundantTypeCheck.h

+//===--- LexicalcastredundanttypeCheck.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_BOOST_LEXICALCASTREDUNDANTTYPE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BOOST_LEXICALCASTREDUNDANTTYPE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace boost {
+
+/// Checks for reduntand, or different type using boost::lexical_cast
+///
+/// Given:
+/// \code
+///   int a = boost::lexical_cast<int>("42");
+///   =>
+///   auto a = boost::lexical_cast<int>("42");
+/// \code
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/boost-LexicalCastRedundantType.html
+class LexicalCastRedundantTypeCheck : public ClangTidyCheck {
+public:
+  LexicalCastRedundantTypeCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void apply(const DeclStmt *D, const std::string &warning);
+};
+
+} // namespace boost
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BOOST_LEXICALCASTREDUNDANTTYPE_H

clang-tidy/boost/LexicalCastRedundantTypeCheck.cpp

+//===--- LexicalcastredundanttypeCheck.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 "LexicalCastRedundantTypeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace boost {
+
+static const std::string LEXICAL_CAST_NAME = "boost::lexical_cast";
+void LexicalCastRedundantTypeCheck::registerMatchers(MatchFinder *Finder) {
+
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      declStmt(has(varDecl(hasInitializer(callExpr(callee(
+                               functionDecl(hasName(LEXICAL_CAST_NAME))))),
+                           unless(hasType(autoType())))))
+          .bind("same_type"),
+      this);
+
+  Finder->addMatcher(
+      declStmt(has(varDecl(hasInitializer(implicitCastExpr(has(callExpr(
+                   callee(functionDecl(hasName(LEXICAL_CAST_NAME))))))))))
+          .bind("different_type"),
+      this);
+}
+
+void LexicalCastRedundantTypeCheck::apply(const DeclStmt *D,
+                                          const std::string &warning) {
+  const auto *Matched = dyn_cast<VarDecl>(*D->decl_begin());
+  if (!Matched)
+    return;
+
+  Matched->getType().dump();
+  SourceRange Range(
+      Matched->getTypeSourceInfo()->getTypeLoc().getSourceRange());
+  // FIXIT: this replace doesnt work
+  diag(Range.getBegin(), warning)
+      << FixItHint::CreateReplacement(Range, "auto ");
+}
+
+void LexicalCastRedundantTypeCheck::check(
+    const MatchFinder::MatchResult &Result) {
+
+  const auto *MatchedSameType = Result.Nodes.getNodeAs<DeclStmt>("same_type");
+  const auto *MatchedDifferentType =
+      Result.Nodes.getNodeAs<DeclStmt>("different_type");
+
+  if (MatchedSameType) {
+    apply(MatchedSameType, "redundant type for lexical_cast. Use auto instead");
+  }
+  if (MatchedDifferentType) {
+    apply(MatchedDifferentType,
+          "different types for lexical_cast. Use auto instead");
+  }
+}
+
+} // namespace boost
+} // namespace tidy
+} // namespace clang

clang-tidy/tool/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   )
 
 add_clang_executable(clang-tidy
   ClangTidyMain.cpp
   )
 target_link_libraries(clang-tidy
   clangAST
   clangASTMatchers
   clangBasic
   clangTidy
+  clangTidyBoostModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
   clangTidyPerformanceModule
   clangTidyReadabilityModule
   clangTooling
   )
 
 install(TARGETS clang-tidy
   RUNTIME DESTINATION bin)
 
 install(PROGRAMS clang-tidy-diff.py DESTINATION share/clang)
 install(PROGRAMS run-clang-tidy.py DESTINATION share/clang)

clang-tidy/tool/ClangTidyMain.cpp

static int clangTidyMain(int argc, const char **argv) {
   return 0;
 }
 
 // This anchor is used to force the linker to link the CERTModule.
 extern volatile int CERTModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED CERTModuleAnchorDestination =
     CERTModuleAnchorSource;
 
+// This anchor is used to force the linker to link the BoostModule.
+extern volatile int BoostModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED BoostModuleAnchorDestination =
+    BoostModuleAnchorSource;
+
 // This anchor is used to force the linker to link the LLVMModule.
 extern volatile int LLVMModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED LLVMModuleAnchorDestination =
     LLVMModuleAnchorSource;
 
 // This anchor is used to force the linker to link the CppCoreGuidelinesModule.
 extern volatile int CppCoreGuidelinesModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =

docs/clang-tidy/checks/boost-LexicalCastRedundantType.rst

+.. title:: clang-tidy - boost-LexicalCastRedundantType
+
+boost-LexicalCastRedundantType
+==============================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =========================
 
 .. toctree::   
+   boost-LexicalCastRedundantType
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-env33-c
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp

test/clang-tidy/boost-LexicalCastRedundantType.cpp

+// RUN: %check_clang_tidy %s boost-lexical-cast-redundant-type %t
+
+namespace boost {
+template <typename T, typename U>
+T lexical_cast(const U &) { return T(); }
+}
+
+void same_type() {
+  // FIXME: Add something that triggers the check here.
+  int a1 = boost::lexical_cast<int>("42");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: redundant type for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: auto a1 = boost::lexical_cast<int>("42");
+
+  long long a2 = boost::lexical_cast<long long>("42");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: redundant type for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: auto a2 = boost::lexical_cast<long long>("42");
+
+  using MyType = unsigned long long;
+  MyType a3 = boost::lexical_cast<MyType>("42");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: redundant type for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: MyType a3 = boost::lexical_cast<MyType>("42");
+
+  unsigned long long a4 = boost::lexical_cast<MyType>("2137");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: redundant type for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: auto a4 = boost::lexical_cast<MyType>("2137");
+
+  // multiple decls
+  int a9 = boost::lexical_cast<int>("42"), a10 = boost::lexical_cast<int>("24");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: redundant type for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: a9 = boost::lexical_cast<int>("42"), a10 = boost::lexical_cast<int>("24");
+
+
+  MyType a11 = boost::lexical_cast<MyType>("42"), a12 = boost::lexical_cast<unsigned long long>("24");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: redundant type for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: auto a11 = boost::lexical_cast<MyType>("42"), a12 = boost::lexical_cast<unsigned long long>("24");
+
+  unsigned long long a13 = boost::lexical_cast<unsigned long long>("42"), a14 = boost::lexical_cast<MyType>("24");
+  // CHECK-FIXES: auto a13 = boost::lexical_cast<unsigned long long>("42"), a14 = boost::lexical_cast<MyType>("24");
+
+  a13 = boost::lexical_cast<unsigned long long>("42");
+
+  int a15 = boost::lexical_cast<int>("42"), a16 = 15;
+  // CHECK-FIXES: auto a13 = boost::lexical_cast<unsigned long long>("42"), a14 = boost::lexical_cast<MyType>("24");
+
+  long long *p = boost::lexical_cast<long long*>("fdas");
+
+}
+
+void different_type() {
+  int a5 = boost::lexical_cast<long long>("1488");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: different types for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: auto a5 = boost::lexical_cast<long long>("1488");
+
+  long long a6 = boost::lexical_cast<int>("2137");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: different types for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: auto a6 = boost::lexical_cast<int>("2137");
+
+  using MyType = unsigned long long;
+  long long a7 = boost::lexical_cast<MyType>("42");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: different types for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: auto a7 = boost::lexical_cast<MyType>("42");
+
+  MyType a8 = boost::lexical_cast<int>("42");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: different types for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+  // CHECK-FIXES: auto a8 = boost::lexical_cast<int>("42");
+
+  // multiple decls with different types - no fixit
+  int a9 = boost::lexical_cast<long long>("42"), a10 = boost::lexical_cast<int>("24");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: different types for lexical_cast. Use auto instead [boost-lexical-cast-redundant-type]
+
+}
+
+// This should not generate any warnings.
+void no_warnings(int x) {
+
+  auto a4 = boost::lexical_cast<unsigned>("-42");
+  auto a5 = boost::lexical_cast<unsigned>(x);
+  auto a6 = boost::lexical_cast<int>("42"), a7 = boost::lexical_cast<int>("24");
+
+  boost::lexical_cast<int>("32");
+  no_warnings(boost::lexical_cast<unsigned>(32));
+}
+
+// FIXME: Verify the applied fix.
+//   * Make the CHECK patterns specific enough and try to make verified lines
+//     unique to avoid incorrect matches.
+
+// FIXME: Add something that doesn't trigger the check here.