boost-use-to-string check

[hokein]

Is there any better solution for the basic_string problem?

The current solution looks fine to me. I don't have a better solution. Maybe @alexfh has.

docs/clang-tidy/checks/boost-use-to-string.rst

7

Please update the document.

test/clang-tidy/boost-use-to-string.cpp

31

Normally you only need to check the whole warning message in the first line.
For others, use prefix message to keep the line short.

[LegalizeAdulthood]

clang-tidy/boost/BoostTidyModule.cpp

20

This should be BoostModule, not ModernizeModule.

clang-tidy/boost/UseToStringCheck.cpp

25

getMatcher doesn't reveal the intent here; the name should reveal what is matched without me having to reverse engineer that information from it's defintiion.

34

Why do we need to match something that is implementation specific? This seems really fragile.

41

Ditto

56–59

Why do we need variables for these strings? They are used only once and the introduced variable name doesn't reveal more intent.

docs/clang-tidy/checks/list.rst

89

Why was this added? It seems unrelated to this changeset.

test/clang-tidy/boost-use-to-string.cpp

5–11

Isn't there an existing stub header file providing std::{w,}string?

77–82

Do we know that the semantics are the same for floating-point types?

[Prazek]

There is also big issue here - I don't check if the type of argument is builtinType. I don't know why but matcher "builtinType" didn't work when I tried to use it. (like I wrote in another email)

clang-tidy/boost/UseToStringCheck.cpp

34

Yes it is, but the problem here is that I can't find better solution:
For example better solution would be to use CXXRecordDecl(hasName("std::basic_string<char>")), because it looks through namespaces, and it doesn't care where std::string was defined. The problem is that I don't know if I can move easly from QualType to CXXRecordDecl - at least I coudn't find it in docs.

docs/clang-tidy/checks/list.rst

89

Yep, I was making to check in the same time. I will split it.

[Prazek]

test/clang-tidy/boost-use-to-string.cpp

77–82

I will check it.

[Prazek]

fixed many things thaks to Alexander

[Prazek]

test/clang-tidy/boost-use-to-string.cpp

5–11

I don't see any test using something like this.

[LegalizeAdulthood]

clang-tidy/boost/UseToStringCheck.cpp

56

Q seems pretty obtuse to me as an identifier. I normally think of template arguments as T, if we are going to use a single letter. Otherwise an intention revealing name would be better. Something like argType perhaps?

[LegalizeAdulthood]

Please summarize this check in docs/ReleaseNotes.rst in the clang-tidy section.

[Prazek]

Updated ReleaseNotes and also fixed bug.
After lgtm please lgtm also this http://reviews.llvm.org/D18274 because I want to send them together, but in separate commits.

[alexfh]

clang-tidy/boost/UseToStringCheck.cpp

56

"Matched" isn't useful here and "ToString" is misleading. I'd name this LexicalCastCall or just Call.

60

When can CharType be isNull()? Do you have a test case for this?

65

I'd rewrite this code as:

StringRef StringType;
if (CharType->isSpecificBuiltinType(BuiltinType::Char_S) ||
    CharType->isSpecificBuiltinType(BuiltinType::Char_U))
  StringType = "string";
else if (CharType->isSpecificBuiltinType(BuiltinType::WChar_S) ||
         CharType->isSpecificBuiltinType(BuiltinType::WChar_U))
  StringType = "wstring";
else
  return;

diag(Call->getLocStart(), "use std::to_%0 instead of boost::lexical_cast<std::%0>")
    << StringType << FixItHint::CreateReplacement(
        CharSourceRange::getCharRange(Call->getLocStart(),
                           Call->getArg(0)->getExprLoc()),
        (llvm::Twine("std::to_") + StringType).str());

69

Please move this comment inside the if body. As it is now, it breaks the formatting.

docs/clang-tidy/checks/boost-use-to-string.rst

7

Please enclose inline code snippets in double backquotes (int, std::string, etc.).

[curdeius]

Minor remark.

docs/clang-tidy/checks/list.rst

7

toctree directive needs the next line to be blank

[alexfh]

FYI, a fix for the assertion failure you see is sent for review as http://reviews.llvm.org/D19144. Let's wait for it to land, before going on with this patch.

[Prazek]

Small tests update. Ping me when your patch will be in trunk

[Prazek]

clang-tidy/boost/UseToStringCheck.cpp

60

I think it's because of some libstdc++ implementation, but without it it was crashing.

[alexfh]

clang-tidy/boost/UseToStringCheck.cpp

54

These comments don't seem to be useful, but if you want to leave them, please surround the bodies with braces, since they are longer than one line.

61

Please add a reduced test case for this.

[alexfh]

FYI, an alternative fix has been submitted in http://reviews.llvm.org/D19231. Please check whether it fixes the issue.

[alexfh]

test/clang-tidy/boost-use-to-string.cpp

3

nit: Remove one empty line.

[Prazek]

clang-tidy/boost/UseToStringCheck.cpp

61

I don't see it crashing right now on the same test when it was crashing before, so I guess it won't be needed

[Prazek]

I hope it is final

[Prazek]

Alex, if you accept this revision, please accept this also
http://reviews.llvm.org/D18274

[alexfh]

clang-tidy/boost/BoostTidyModule.cpp

26

This method doesn't do anything, please remove it.

clang-tidy/boost/UseToStringCheck.cpp

38

Why do you need hasParent() around isInTemplateInstantiation()?

69

No need for the second llvm::Twine.

docs/clang-tidy/checks/boost-use-to-string.rst

13

Please remove one empty line.

[alexfh]

Looks good with a couple of nits. Please submit this as a single patch, there's no need to add an empty "boost" module separately.

clang-tidy/boost/UseToStringCheck.cpp

39

clang-format?

clang-tidy/boost/UseToStringCheck.h

20

1. Please enclose inline code snippets in backquotes.
2. s/Matches/Finds/ or Flags
3. s/and replaces it/and replaces them
4. s/to_string/std::to_string/, same for to_wstring.

[Prazek]

clang-tidy/boost/UseToStringCheck.cpp

39

nope, everything was formated

[Prazek]

merged with boost module

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 "UseToStringCheck.h"
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace boost {
 
 class BoostModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<UseToStringCheck>("boost-use-to-string");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     return Options;
   }
 };
 
 // Register the BoostModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<BoostModule> 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
+  UseToStringCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/boost/UseToStringCheck.h

+//===--- UseToStringCheck.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_USE_TO_STRING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BOOST_USE_TO_STRING_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace boost {
+
+/// Matches calls to boost::lexical_cast<std::string> and
+/// boost::lexical_cast<std::wstring> and replaces it with to_string and
+/// to_wstring calls.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/boost-use-to-string.html
+class UseToStringCheck : public ClangTidyCheck {
+public:
+  UseToStringCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void apply(const CallExpr *MatchedExpr, const std::string &message,
+             const std::string &replacement);
+};
+
+} // namespace boost
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BOOST_USE_TO_STRING_H

clang-tidy/boost/UseToStringCheck.cpp

+//===--- UseToStringCheck.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 "UseToStringCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace boost {
+
+AST_MATCHER(Type, isStrictlyInteger) {
+  return Node.isIntegerType() && !Node.isAnyCharacterType() &&
+         !Node.isBooleanType();
+}
+
+void UseToStringCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      callExpr(
+          hasDeclaration(functionDecl(
+              returns(hasDeclaration(classTemplateSpecializationDecl(
+                  hasName("std::basic_string"),
+                  hasTemplateArgument(0,
+                                      templateArgument().bind("char_type"))))),
+              hasName("boost::lexical_cast"),
+              hasParameter(0, hasType(qualType(has(
+                                  substTemplateTypeParmType(isStrictlyInteger())
+                                      .bind("t"))))))),
+          argumentCountIs(1)) //, unless(isInTemplateInstantiation()))
+          .bind("to_string"),
+      this);
+}
+
+void UseToStringCheck::apply(const CallExpr *MatchedExpr,
+                             const std::string &message,
+                             const std::string &replacement) {
+  auto *Argument = MatchedExpr->getArg(0);
+
+  diag(MatchedExpr->getLocStart(), message) << FixItHint::CreateReplacement(
+      CharSourceRange::getCharRange(MatchedExpr->getLocStart(),
+                                    Argument->getExprLoc()),
+      replacement);
+}
+
+void UseToStringCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedToString = Result.Nodes.getNodeAs<CallExpr>("to_string");
+  auto Q = Result.Nodes.getNodeAs<TemplateArgument>("char_type")->getAsType();
+
+  if (Q.isNull())
+    return;
+
+  if (Q->isSpecificBuiltinType(BuiltinType::Char_S) ||
+      Q->isSpecificBuiltinType(BuiltinType::Char_U)) {
+    apply(MatchedToString,
+          "use std::to_string instead of boost::lexical_cast<std::string>",
+          "std::to_string(");
+  }
+  // Is Q 'wchar_t'
+  else if (Q->isSpecificBuiltinType(BuiltinType::WChar_S) ||
+           Q->isSpecificBuiltinType(BuiltinType::WChar_U)) {
+    apply(MatchedToString,
+          "use std::to_wstring instead of boost::lexical_cast<std::wstring>",
+          "std::to_wstring(");
+  }
+}
+
+} // namespace boost
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/boost-use-to-string.rst

+.. title:: clang-tidy - boost-use-to-string
+
+boost-use-to-string
+===================
+
+This check finds conversion from integer type like int to std::string or
+std::wstring using boost::lexical_cast, and replace it to calls of
+std::to_string and std::to_wstring.
+
+It doesn't replace conversion from floating points despite the to_string
+overloads, because it would change the behaviour.
+
+
+  .. code-block:: c++
+    - auto str = boost::lexical_cast<std::string>(42);
+    + auto str = std::to_string(42);
+
+    - auto wstr = boost::lexical_cast<std::wstring>(2137LL);
+    + auto wstr = std::to_wstring(2137LL);
+

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =========================
 
 .. toctree::   
+   boost-use-to-string
    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
.. toctree::
    modernize-pass-by-value
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
    modernize-shrink-to-fit
    modernize-use-auto
    modernize-use-default
    modernize-use-nullptr
    modernize-use-override
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop
    readability-braces-around-statements
    readability-container-size-empty
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-named-parameter
    readability-redundant-control-flow
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-uniqueptr-delete-release

test/clang-tidy/boost-use-to-string.cpp

+// RUN: %check_clang_tidy %s boost-use-to-string %t
+
+
+namespace std {
+
+template <typename T> class basic_string {};
+
+using string = basic_string<char>;
+using wstring = basic_string<wchar_t>;
+}
+
+namespace boost {
+template <typename T, typename V>
+T lexical_cast(const V&) {
+  return T();
+};
+}
+
+struct my_weird_type {};
+
+std::string fun(const std::string &) {}
+
+void test_to_string1() {
+
+  auto xa = boost::lexical_cast<std::string>(5);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use std::to_string instead of boost::lexical_cast<std::string> [boost-use-to-string]
+// CHECK-FIXES: auto xa = std::to_string(5);
+
+ auto z = boost::lexical_cast<std::string>(42LL);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use std::to_string {{..}}
+// CHECK-FIXES: auto z = std::to_string(42LL);
+
+  // this should not trigger
+  fun(boost::lexical_cast<std::string>(42.0));
+  auto non = boost::lexical_cast<my_weird_type>(42);
+  boost::lexical_cast<int>("12");
+}
+
+void test_to_string2() {
+  int a;
+  long b;
+  long long c;
+  unsigned d;
+  unsigned long e;
+  unsigned long long f;
+  float g;
+  double h;
+  long double i;
+  bool j;
+
+  fun(boost::lexical_cast<std::string>(a));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_string {{..}}
+// CHECK-FIXES: fun(std::to_string(a));
+  fun(boost::lexical_cast<std::string>(b));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_string {{..}}
+// CHECK-FIXES: fun(std::to_string(b));
+  fun(boost::lexical_cast<std::string>(c));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_string {{..}}
+// CHECK-FIXES: fun(std::to_string(c));
+  fun(boost::lexical_cast<std::string>(d));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_string {{..}}
+// CHECK-FIXES: fun(std::to_string(d));
+  fun(boost::lexical_cast<std::string>(e));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_string {{..}}
+// CHECK-FIXES: fun(std::to_string(e));
+  fun(boost::lexical_cast<std::string>(f));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_string {{..}}
+// CHECK-FIXES: fun(std::to_string(f));
+
+  // No change for floating numbers.
+  fun(boost::lexical_cast<std::string>(g));
+  fun(boost::lexical_cast<std::string>(h));
+  fun(boost::lexical_cast<std::string>(i));
+  // And bool.
+  fun(boost::lexical_cast<std::string>(j));
+}
+
+std::string fun(const std::wstring &) {}
+
+void test_to_wstring() {
+  int a;
+  long b;
+  long long c;
+  unsigned d;
+  unsigned long e;
+  unsigned long long f;
+  float g;
+  double h;
+  long double i;
+  bool j;
+
+  fun(boost::lexical_cast<std::wstring>(a));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_wstring instead of boost::lexical_cast<std::wstring> [boost-use-to-string]
+// CHECK-FIXES: fun(std::to_wstring(a));
+  fun(boost::lexical_cast<std::wstring>(b));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_wstring {{..}}
+// CHECK-FIXES: fun(std::to_wstring(b));
+  fun(boost::lexical_cast<std::wstring>(c));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_wstring {{..}}
+// CHECK-FIXES: fun(std::to_wstring(c));
+  fun(boost::lexical_cast<std::wstring>(d));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_wstring {{..}}
+// CHECK-FIXES: fun(std::to_wstring(d));
+  fun(boost::lexical_cast<std::wstring>(e));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_wstring {{..}}
+// CHECK-FIXES: fun(std::to_wstring(e));
+  fun(boost::lexical_cast<std::wstring>(f));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_wstring {{..}}
+// CHECK-FIXES: fun(std::to_wstring(f));
+
+  // No change for floating numbers
+  fun(boost::lexical_cast<std::wstring>(g));
+  fun(boost::lexical_cast<std::wstring>(h));
+  fun(boost::lexical_cast<std::wstring>(i));
+  // and bool.
+  fun(boost::lexical_cast<std::wstring>(j));
+}
+
+template <typename T>
+void string_as_T() {
+  boost::lexical_cast<std::string>(42);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use std::to_string{{..}}
+// CHECK-FIXES: std::to_string(42);
+  // FIXME it should not replace this call
+  //boost::lexical_cast<T>(42);
+}
+
+
+void no_warnings() {
+  fun(boost::lexical_cast<std::string>("abc"));
+  fun(boost::lexical_cast<std::wstring>("abc"));
+  fun(boost::lexical_cast<std::string>(my_weird_type{}));
+  string_as_T<int>();
+  string_as_T<std::string>();
+}
+
+
+