[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_common.cpp:99
+
+static INLINE bool ReportSupportsColors() {
+  return report_file.SupportsColors();
----------------
INLINE is not necessary

can this be?
```
return SANITIZER_FUCHSIA || report_file.SupportsColors();
```



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp:58
+  // well as whatever one is currently-active and where the string to print it
+  // is located in the buffer.
+  DecorationKind prev = kUnknown, next = kUnknown;
----------------
should these be kDefault


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp:59
+  // is located in the buffer.
+  DecorationKind prev = kUnknown, next = kUnknown;
+  do {
----------------
```
DecorationKind prev = kUnknown;
DecorationKind next = kUnknown;
```


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp:69
+        code = internal_simple_strtoll(in + 2, &endptr, 10);
+      if (endptr <= in + 2 || *endptr != 'm')
+        code = kUnknown;
----------------
code uses "code = kUnknown;" to only break the loop
if we wrap this into function:

```

const char* getCode(const char* &in, DecorationKind &next) {
while (true) {
    if (internal_strncmp(in, "\033[", 2) != 0)
      return in;
    const char* endptr = in + 2;
    s64 code = internal_simple_strtoll(endptr, &endptr, 10);
    if (endptr <= in + 2 || *endptr++ != 'm')
       return in;

    switch (code) {
        case kDefault:
        case kBold:
          break;
        case kBlack:
        case kRed:
        case kGreen:
        case kYellow:
        case kBlue:
        case kMagenta:
        case kCyan:
        case kWhite:
          if (next == kBold)
            break;
        default:
           return in;
    }
    next = static_cast<DecorationKind>(code);
}
}
```

we don't need kUnknown at all


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp:83
+        case kWhite:
+          if (next != kBold)
+            code = kUnknown;
----------------
why next must be kBold?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp:91
+        break;
+      in = endptr + 1;
+      next = static_cast<DecorationKind>(code);
----------------
increment "endptr" right after != 'm' check, so we know where 1 is from


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp:99
+      auto tag = ToStr(next);
+      auto tag_len = internal_strlen(tag);  // Not null-terminated!
+      internal_strncpy(out, tag, tag_len);
----------------
why? // Not null-terminated!
ToStr returns regular c-strings


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

https://reviews.llvm.org/D63179