[PATCH] D15411: [clang-tidy] Check for suspicious string assignments.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

That's quite a surprising behavior. Looks like a bug in the library implementation (or in the standard) to me.

But the check is awesome. Thank you!


================
Comment at: clang-tidy/misc/StringAssignmentCheck.cpp:21
@@ +20,3 @@
+void StringAssignmentCheck::registerMatchers(MatchFinder *Finder) {
+  auto Matcher = cxxOperatorCallExpr(
+      anyOf(hasOverloadedOperatorName("="), hasOverloadedOperatorName("+=")),
----------------
Please only register the matcher for C++ code (see almost any other check for an example). It also seems that you don't need the `Matcher` variable.

================
Comment at: clang-tidy/misc/StringAssignmentCheck.cpp:33
@@ +32,3 @@
+
+  if (!ArgType->isIntegerType() || ArgType->isAnyCharacterType())
+    return;
----------------
The `isIntegerType()` check can be moved to the matcher. I'd also add a matcher for `isAnyCharacterType` and move that check to the matcher as well. The latter is fine for a follow-up.

================
Comment at: clang-tidy/misc/StringAssignmentCheck.cpp:36
@@ +35,3 @@
+  SourceLocation Loc = Argument->getLocStart();
+  SmallVector<FixItHint, 2> Hints;
+  if (!Loc.isMacroID()) {
----------------
Instead of storing the fixits in an array, I'd organize the code as follows:

  auto Diag = diag(...);
  if (!Loc.isMacroID()) {
    Diag << FixItHint::CreateInsertion(...) << FixItHint::CreateInsertion(...);
  }

================
Comment at: clang-tidy/misc/StringAssignmentCheck.cpp:38
@@ +37,3 @@
+  if (!Loc.isMacroID()) {
+    Hints.push_back(FixItHint::CreateInsertion(Loc, "std::to_string("));
+    Hints.push_back(FixItHint::CreateInsertion(
----------------
Please check that we're in C++11 mode before suggesting this fix.

================
Comment at: clang-tidy/misc/StringAssignmentCheck.h:1
@@ +1,2 @@
+//===--- StringAssignmentCheck.h - clang-tidy--------------------*- C++ -*-===//
+//
----------------
I suggest making the name more specific, e.g. `StringIntegerAssignmentCheck`. Same for the check name (`misc-string-integer-assignment`).


http://reviews.llvm.org/D15411