Diff Detail
Event Timeline
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. |
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: | |
docs/clang-tidy/checks/list.rst | ||
101 | Yep, I was making to check in the same time. I will split it. |
test/clang-tidy/boost-use-to-string.cpp | ||
---|---|---|
77–82 | I will check it. |
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? |
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.
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.). |
Minor remark.
docs/clang-tidy/checks/list.rst | ||
---|---|---|
7 | toctree directive needs the next line to be blank |
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.
clang-tidy/boost/UseToStringCheck.cpp | ||
---|---|---|
60 | I think it's because of some libstdc++ implementation, but without it it was crashing. |
FYI, an alternative fix has been submitted in http://reviews.llvm.org/D19231. Please check whether it fixes the issue.
test/clang-tidy/boost-use-to-string.cpp | ||
---|---|---|
3 | nit: Remove one empty line. |
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 |
Alex, if you accept this revision, please accept this also
http://reviews.llvm.org/D18274
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. |
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 |
|
clang-tidy/boost/UseToStringCheck.cpp | ||
---|---|---|
39 | nope, everything was formated |
This should be BoostModule, not ModernizeModule.