[PATCH] [LSan] Common leak checking module.
Alexander Potapenko
glider at google.com
Thu May 16 04:32:34 PDT 2013
================
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
More information about the llvm-commits
mailing list