Page MenuHomePhabricator

boost-use-to-string check
ClosedPublic

Authored by Prazek on Mar 13 2016, 2:34 PM.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 50558.Mar 13 2016, 2:34 PM
Prazek retitled this revision from to boost-use-to-string check.
Prazek updated this object.
Prazek added a reviewer: alexfh.
Prazek added a subscriber: cfe-commits.
hokein added a subscriber: hokein.Mar 14 2016, 7:26 AM

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 added inline comments.
clang-tidy/boost/BoostTidyModule.cpp
21

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
101

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?

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
101

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

Prazek added inline comments.Mar 14 2016, 10:59 AM
test/clang-tidy/boost-use-to-string.cpp
77–82

I will check it.

Prazek updated this revision to Diff 51057.Mar 18 2016, 12:11 PM
Prazek updated this object.

fixed many things thaks to Alexander

Prazek marked 12 inline comments as done.Mar 18 2016, 12:12 PM
Prazek added inline comments.Mar 18 2016, 12:18 PM
test/clang-tidy/boost-use-to-string.cpp
6–12

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

clang-tidy/boost/UseToStringCheck.cpp
57

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?

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

Prazek updated this revision to Diff 52378.Apr 1 2016, 9:06 AM
Prazek updated this object.

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.

Prazek marked an inline comment as done.Apr 1 2016, 9:08 AM
alexfh requested changes to this revision.Apr 1 2016, 7:22 PM
alexfh edited edge metadata.
alexfh added inline comments.
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.).

This revision now requires changes to proceed.Apr 1 2016, 7:22 PM

Minor remark.

docs/clang-tidy/checks/list.rst
7

toctree directive needs the next line to be blank

Prazek updated this revision to Diff 53121.Apr 9 2016, 6:37 AM
Prazek updated this object.
Prazek edited edge metadata.
Prazek marked 5 inline comments as done.
alexfh requested changes to this revision.Apr 15 2016, 4:55 AM
alexfh edited edge metadata.

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.

This revision now requires changes to proceed.Apr 15 2016, 4:55 AM
Prazek updated this revision to Diff 53891.Apr 15 2016, 8:21 AM
Prazek updated this object.
Prazek edited edge metadata.

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

Prazek added inline comments.Apr 16 2016, 10:11 AM
clang-tidy/boost/UseToStringCheck.cpp
60

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

alexfh added inline comments.Apr 18 2016, 6:51 AM
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 requested changes to this revision.Apr 18 2016, 6:51 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 18 2016, 6:51 AM

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

alexfh added inline comments.Apr 19 2016, 3:10 PM
test/clang-tidy/boost-use-to-string.cpp
3

nit: Remove one empty line.

Prazek marked 5 inline comments as done.Apr 26 2016, 12:53 PM
Prazek added inline comments.
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 updated this revision to Diff 55077.Apr 26 2016, 1:04 PM
Prazek edited edge metadata.
Prazek marked an inline comment as done.

I hope it is final

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

alexfh requested changes to this revision.Apr 27 2016, 4:35 AM
alexfh edited edge metadata.
alexfh added inline comments.
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.

This revision now requires changes to proceed.Apr 27 2016, 4:35 AM
Prazek marked 4 inline comments as done.Apr 27 2016, 12:04 PM
Prazek updated this revision to Diff 55280.Apr 27 2016, 12:08 PM
Prazek edited edge metadata.
alexfh accepted this revision.Apr 28 2016, 12:00 PM
alexfh edited edge metadata.

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.
This revision is now accepted and ready to land.Apr 28 2016, 12:00 PM
Prazek marked 2 inline comments as done.Apr 29 2016, 3:02 AM
Prazek added inline comments.
clang-tidy/boost/UseToStringCheck.cpp
39

nope, everything was formated

Prazek updated this revision to Diff 55558.Apr 29 2016, 3:12 AM
Prazek edited edge metadata.
Prazek marked an inline comment as done.

merged with boost module

Prazek closed this revision.Apr 29 2016, 12:07 PM