[PATCH] D21447: Comprehensive static instrumentation (3/3): Runtime and tests

[Kostya Serebryany via llvm-commits]

kcc added inline comments.

================
Comment at: lib/csi/csi.h:6
@@ +5,3 @@
+
+#ifdef __cplusplus
+#define EXTERN_C extern "C" {
----------------
does it really need to be C-compatible? 
This complicates the interface. 

================
Comment at: lib/csi/csi.h:7
@@ +6,3 @@
+#ifdef __cplusplus
+#define EXTERN_C extern "C" {
+#define EXTERN_C_END }
----------------
avoid extra defines in the interface headers, if you can.

================
Comment at: lib/csi/csi.h:15
@@ +14,3 @@
+
+#define WEAK __attribute__((weak))
+
----------------
CSI_WEAK

================
Comment at: lib/csi/csi.h:29
@@ +28,3 @@
+
+#define UNKNOWN_CSI_ID ((csi_id_t)-1)
+
----------------
avoid defines. 
not so easy in C, easy in C++

================
Comment at: lib/csi/csi.h:42
@@ +41,3 @@
+
+WEAK void __csi_unit_init(const char * const file_name,
+                          const instrumentation_counts_t counts);
----------------
watch for spacing around "*"

================
Comment at: lib/csi/csi.h:80
@@ +79,3 @@
+    // TODO(ddoucet): Why is this 32 bits?
+    int32_t line_number;
+    char *filename;
----------------
the docs show this struct differently. 

================
Comment at: lib/csi/csi.h:100
@@ +99,2 @@
+
+#endif
----------------
#endif  // __CSI_H__

================
Comment at: lib/csi/csirt.c:18
@@ +17,3 @@
+    source_loc_t *entries;
+} fed_table_t;
+
----------------
Please follow the llvm style guides for var/type/field names.

The naming of the interface functions (and types?) is fine, please change the rest.  

================
Comment at: lib/csi/csirt.c:54
@@ +53,3 @@
+static void initialize_fed_tables() {
+    fed_tables = (fed_table_t *)malloc(NUM_FED_TYPES * sizeof(fed_table_t));
+    assert(fed_tables != NULL);
----------------
use C++ (new).
But also why do you need to allocate a fixed size buffer? 

================
Comment at: lib/csi/csirt.c:59
@@ +58,3 @@
+        table.num_entries = 0;
+        table.entries = NULL;
+        fed_tables[i] = table;
----------------
nullptr, here and everywhere

================
Comment at: lib/csi/csirt.c:74
@@ +73,3 @@
+    if (total_num_entries > 0) {
+        table->entries = (source_loc_t *)realloc(table->entries,
+                                                 total_num_entries * sizeof(source_loc_t));
----------------
I hate realloc. 
Not sure if it can be *easily* replaced here. 

================
Comment at: lib/csi/csirt.c:82
@@ +81,3 @@
+// Add a new FED table of the given type.
+static inline void add_fed_table(fed_type_t fed_type, int64_t num_entries, source_loc_t const * fed_entries) {
+    ensure_fed_table_capacity(fed_type, num_entries);
----------------
watch for 80 chars

================
Comment at: lib/csi/csirt.c:86
@@ +85,3 @@
+    csi_id_t base = table->num_entries - num_entries;
+    for (csi_id_t i = 0; i < num_entries; i++) {
+        table->entries[base + i] = fed_entries[i];
----------------
watch for {} and single-line statements. 


Repository:
  rL LLVM

http://reviews.llvm.org/D21447