[PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Felix Berger via cfe-commits]

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