[PATCH] D15032: [clang-tidy] new checker cppcoreguidelines-pro-lifetime

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this! A few initial nits (I don't have the chance to complete the review yet, but wanted to get you some early feedback).


================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeCheck.cpp:34-35
@@ +33,4 @@
+
+  Finder->addMatcher(functionDecl().bind("fun"), this);
+  Finder->addMatcher(varDecl(hasGlobalStorage()).bind("globalVar"), this);
+}
----------------
These can be combined into a single matcher using anyOf() (should be slightly more efficient than registering two separate matchers).

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeCheck.h:18
@@ +17,3 @@
+
+/// FIXME: Write a short description.
+///
----------------
Should write that short description. :-)

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:94-97
@@ +93,6 @@
+  Pointer(const FieldDecl *FD) : FD(FD) { assert(FD); }
+  Pointer(Pointer &&) = default;
+  Pointer &operator=(Pointer &&) = default;
+  Pointer(const Pointer &) = default;
+  Pointer &operator=(const Pointer &) = default;
+
----------------
I don't believe there's a need to explicitly default these (here and elsewhere), so they should just be removed.

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:99-101
@@ +98,5 @@
+
+  bool operator==(const Pointer &o) const { return VD == o.VD && FD == o.FD; }
+  bool operator!=(const Pointer &o) const { return !(*this == o); }
+  bool operator<(const Pointer &o) const {
+    if (VD != o.VD)
----------------
Should be `O` instead of `o` per our usual conventions, here and elsewhere.

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:103-104
@@ +102,4 @@
+    if (VD != o.VD)
+      return VD < o.VD;
+    return FD < o.FD;
+  }
----------------
I would feel more comfortable using `std::les<>` for this to avoid comparisons between unrelated pointers not having a total ordering.

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:121
@@ +120,3 @@
+  /// Returns true if this pointer is a member variable of the class of the
+  /// current method
+  bool isMemberVariable() const { return FD; }
----------------
Comments should be properly punctuated (here and elsewhere).

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:153
@@ +152,3 @@
+
+  // static bool is(const ValueDecl *VD) { return get(VD).hasValue(); }
+
----------------
Should remove this instead of leaving it commented out.

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:178
@@ +177,3 @@
+class Owner {
+  enum SpecialType { VARDECL = 0, NULLPTR = 1, STATIC = 2, TEMPORARY = 3 };
+
----------------
Don't use all caps for enumerators (that's generally reserved for just macros by our conventions), here and elsewhere.

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:229
@@ +228,3 @@
+      return Pointer::get(VD.getPointer());
+    return Optional<Pointer>();
+  }
----------------
I think this should return None instead.

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:548-549
@@ +547,4 @@
+  /// IgnoreParenImpCasts - Ignore parentheses and implicit casts.  Strip off
+  /// any ParenExpr
+  /// or ImplicitCastExprs, returning their operand.
+  /// Does not ignore MaterializeTemporaryExpr as Expr::IgnoreParenImpCasts
----------------
Reformat the comment to fit a bit better?

================
Comment at: clang-tidy/cppcoreguidelines/ProLifetimeVisitor.cpp:552
@@ +551,3 @@
+  /// would.
+  const Expr *IgnoreParenImpCasts(const Expr *E) {
+    while (true) {
----------------
Method can be `const` qualified.


http://reviews.llvm.org/D15032