[PATCH] D91893: [clang-tidy] performance-unnecessary-copy-initialization: Prevent false positives when dependent variable is modified.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I have some quick drive-by comments but still have to think about the test cases a bit more.



================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:74
+// the BlockStmt. It does this by checking the following:
+// 1. If the variable is neither a reference nor a pointer then the the
+// isOnlyUsedAsConst() check is sufficient.
----------------
typo: the the


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:77
+// 2. If the (reference or pointer) variable is not initialized in a DeclStmt in
+// the BlockStmt. In this case it its pointee is likely not modified (unless it
+// is passed as an alias into the method as well).
----------------
typo: it its


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:83
+// object arg or variable that is referenced is immutable as well.
+bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
+                                     const Stmt &BlockStmt,
----------------
You should mark the function as being `static`.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:86
+                                     ASTContext &Context) {
+  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context)) {
+    return false;
----------------
Elide braces around single-line `if` statements. For the cases involving comments, I'd recommend hoisting the comments above the if when returning a constant.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp:90
+  QualType T = InitializingVar.getType();
+  if (!llvm::dyn_cast<ReferenceType>(T) && !llvm::dyn_cast<PointerType>(T)) {
+    // The variable is a value type and we know it is only used as const. Safe
----------------
No need for the `llvm::`, is there? This should be replaced with `!isa<ReferenceType, PointerType>(T)` since you only care about the Boolean results.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91893/new/

https://reviews.llvm.org/D91893