[PATCH] [LSan] Common leak checking module.

[Alexander Potapenko]

================
Comment at: lib/lsan/lsan_common.h:71
@@ +70,3 @@
+extern Flags lsan_flags;
+inline Flags *flags() { return &lsan_flags; }
+
----------------
Why not declare this method inside the Flags declaration?

================
Comment at: lib/lsan/lsan_common.h:159
@@ +158,3 @@
+class LsanMetadata {
+  void *metadata_;
+ public:
----------------
Methods before data members, please.

================
Comment at: lib/lsan/lsan_common.cc:34
@@ +33,3 @@
+  f->max_leaks = 0;
+
+  f->log_pointers = false;
----------------
Spare newline?

================
Comment at: lib/lsan/lsan_common.cc:344
@@ +343,3 @@
+    }
+  Leak leak = { 1, leaked_size, stack_trace_id, is_directly_leaked };
+  leaks_.push_back(leak);
----------------
/*hit_count*/1

================
Comment at: lib/lsan/lsan_common.h:25
@@ +24,3 @@
+// Chunk tags.
+enum {
+  TAG_DIRECTLY_LEAKED = 0,  // default
----------------
Please name this enum and use the enum type instead of uptr when operating tags.

================
Comment at: lib/lsan/lsan_common.cc:334
@@ +333,3 @@
+
+void LeakReport::Add(u32 stack_trace_id, uptr leaked_size, uptr tag) {
+  CHECK(tag == TAG_DIRECTLY_LEAKED || tag == TAG_INDIRECTLY_LEAKED);
----------------
s/uptr tag/enum tag_type tag/
or something like that.

================
Comment at: lib/lsan/lsan_common.cc:348
@@ +347,3 @@
+
+static inline bool IsLarger(const Leak &leak1, const Leak &leak2) {
+  return leak1.total_size > leak2.total_size;
----------------
You're passing a pointer to this function into InternalSort. Why make it inline?

================
Comment at: lib/lsan/lsan_common.h:121
@@ +120,3 @@
+
+  LeakReport *leak_report_;
+};
----------------
Either name it "leak_report" without the trailing underscore, or make it private. Ditto for other callback classes.

================
Comment at: lib/lsan/lsan_common.cc:53
@@ +52,3 @@
+
+void InitCommonLsan() {
+  InitializeFlags();
----------------
Is this function used anywhere?
Also, why "Common"?

================
Comment at: lib/lsan/lsan_common.cc:64
@@ +63,3 @@
+#ifdef __x86_64__
+  return ((p >> 47) == 0);
+#else
----------------
Please clarify in a comment why you're checking for the upper 17 bits to be zeroes.

================
Comment at: lib/lsan/lsan_common.h:88
@@ +87,3 @@
+class LeakReport {
+  InternalVector<Leak> leaks_;
+
----------------
Methods before data members, public before private.


http://llvm-reviews.chandlerc.com/D787