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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
