flx added a comment.
I don't have access, so that'd be great if you or Alex could submit the patch,
thanks!
================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:11
@@ -10,2 +10,3 @@
#include "MoveConstructorInitCheck.h"
+#include "../utils/Matchers.h"
#include "clang/AST/ASTContext.h"
----------------
aaron.ballman wrote:
> Errr, I'm not certain that we typically use relative paths for this sort of
> thing. I see we do use relative paths for other things in clang-tidy, but
> this is definitely an uncommon thing in the rest of the code base (outside of
> test or example code).
>
> Not saying you should change anything here, but we may want to consider
> changing the paths at some point so we are more in line with the rest of the
> LLVM projects in not using relative include paths.
Leaving this as is for now as all other ClangTidy specific includes I could
find were relative as well. Not sure if non-relative includes require extra
work in the build rules.
================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:96
@@ +95,3 @@
+
+void MoveConstructorInitCheck::handleMoveConstructor(
+ const MatchFinder::MatchResult &Result) {
----------------
alexfh wrote:
> Other checks have separate options for the include style (see
> `PassByValueCheck.cpp`, for example), so this one should have its own option
> too. If we ever decide to introduce a common option instead, it will be
> easier to do this, if all usages of the `IncludeInserter` do the same thing
> with the include style.
>
> One thing I would change though, is to add the IncludeStyle <-> string
> conversion functions instead of doing this in each check.
Done. I moved the conversion into IncludeSorter where the the enum is defined.
================
Comment at: clang-tidy/modernize/PassByValueCheck.cpp:121
@@ -120,4 +120,3 @@
: ClangTidyCheck(Name, Context),
- IncludeStyle(Options.get("IncludeStyle", "llvm") == "llvm" ?
- IncludeSorter::IS_LLVM : IncludeSorter::IS_Google) {}
+ IncludeStyle(IncludeSorter::toIncludeStyle(Options.get("IncludeStyle",
"llvm"))) {}
----------------
alexfh wrote:
> clang-format?
This is how clang-format formatted it. I used the following invocation:
clang-format --style=LLVM MoveConstructorInitCheck.cpp > formatted
And then diffed to only update format changes in code I added.
================
Comment at: clang-tidy/utils/IncludeSorter.h:29
@@ +28,3 @@
+ // Converts "llvm" to IS_LLVM, otherwise returns IS_Google.
+ static IncludeStyle toIncludeStyle(const std::string &Value);
+
----------------
alexfh wrote:
> nit: The current name gives no hints at what is being converted to
> IncludeStyle, but `parseIncludeStyle` would make it clear that the source is
> a string.
Done, although the argument type uniquely identifies a function.
================
Comment at: clang-tidy/utils/Matchers.h:1
@@ +1,2 @@
+//===--- Matchers.h -
clang-tidy-------------------------------------------===//
+//
----------------
aaron.ballman wrote:
> You are missing header include guards.
Thanks for catching that. Done.
================
Comment at: clang-tidy/utils/TypeTraits.cpp:32
@@ +31,3 @@
+ // Ignore trivially copyable types.
+ return !(Type->isScalarType() || Type.isTriviallyCopyableType(Context) ||
+ classHasTrivialCopyAndDestroy(Type));
----------------
aaron.ballman wrote:
> No need to check for isScalarType() because isTriviallyCopyableType() already
> does that correctly.
Done. Also demorganized the expression to make it easier to understand.
================
Comment at: clang-tidy/utils/TypeTraits.h:22
@@ +21,3 @@
+/// destructor.
+bool classHasTrivialCopyAndDestroy(QualType Type);
+
----------------
aaron.ballman wrote:
> I think this is an implementation detail more than a trait we want to expose.
Removed from header.
http://reviews.llvm.org/D12839
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
