[PATCH] D63179: [sanitizer-common] Reduce ANSI color sequences that have no effect.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc:22
+SanitizerCommonDecorator::ToKind(const char *s) {
+  if (internal_strncmp(s, "\033[", 2) != 0 || s[2] == '\0' || s[3] != 'm')
+    return kUnknown;
----------------
lets consume chars to simplify indexing 

```
static const char* getCode(const char* s, char* code) {
    if (internal_strncmp(s, "\033[", 2) != 0 || s[2] == '\0' || s[3] != 'm') 
      return nullptr;
   *code = s[2];
    return s + 4;
}
SanitizerCommonDecorator::ToKind(const char *s) {
  char c;
  if (!(s = getCode(s, &c))
    return kUnknown;

  switch(c)
   ....

  if (!(s = getCode(s, &c))
    return kBold;

 switch(c) {
     ....

}
  
```


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc:89
+  case kBlack:
+  case kRed:
+  case kGreen:
----------------
please remove SanitizerCommonDecorator::StrLen or replace with internal_strln( ToKind() ) 


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc:107
+    char* z = str;
+
+    // We need to keep track of whatever decoration tag we last printed with, as
----------------
z->out?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc:129
+        // non-whitespace character. Print the tag(s) before the character.
+        if (0x20 < *s && *s < 0x7f && prev != next) {
+            n = SanitizerCommonDecorator::StrLen(next);
----------------
why do you need to check for "*s < 0x7f"


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc:138
+        // We have actual characters; print them.
+        if (z != s)
+            *z = *s;
----------------
 if (z != s) can be omitted


================
Comment at: compiler-rt/lib/sanitizer_common/tests/sanitizer_printf_test.cc:196
+ public:
+  // Returns an ASNI escape sequence for 'R' or 'B' as described above.
+  const char* ToStr(char c) {
----------------
ASNI -> ANSI


================
Comment at: compiler-rt/lib/sanitizer_common/tests/sanitizer_printf_test.cc:225
+  int n;
+  for (size_t i = 0; i < sizeof(tests) / sizeof(tests[0]); ++i) {
+    const char* v = tests[i].verbose;
----------------
please use Value-Parameterized Test instead of the loop
https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#specifying-names-for-value-parameterized-test-parameters


================
Comment at: compiler-rt/lib/sanitizer_common/tests/sanitizer_printf_test.cc:229
+    for (size_t j=0; j < strlen(v); j++) {
+      n = snprintf(verbose + off, sizeof(verbose) - off, "%s", d.ToStr(v[j]));
+      ASSERT_GE(n, 0);
----------------
this test logic is quite fancy, it's going to be harder to debug when it detected a bug
I would prefer simple tests where easy to see from the code what was actual input and expected output:

```
SanitizerCommonDecorator::Compact(GetParam().input);
EXPECT_STREQ(GetParam().input, GetParam().expected);
```



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63179/new/

https://reviews.llvm.org/D63179