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

[Aaron Green via Phabricator via llvm-commits]

aarongreen marked an inline comment as done.
aarongreen 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;
----------------
vitalybuka wrote:
> 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) {
>      ....
> 
> }
>   
> ```
That doesn't quite work: the colors are 2 chars, i.e. 31.

I've made getCode use internal_simple_strtoll to return an s64 instead, and advance a const char **s_ptr. That way, each call consumes a '\033[...m' sequence and returns the number represented by '...'

I can't promise it's faster, but hopefully it's clearer?


================
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);
----------------
vitalybuka wrote:
> why do you need to check for "*s < 0x7f"
0x7f is DEL, and above that isn't ascii. Printable chars should be 0x21 (!) to 0x7e (~).


================
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) {
----------------
vitalybuka wrote:
> ASNI -> ANSI
seplling is hrad.


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

https://reviews.llvm.org/D63179