[PATCH] D41290: [YAML] Add support for non-printable characters

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

LGTM too with some suggestions/nitpicks:



================
Comment at: include/llvm/Support/YAMLTraits.h:503-565
+inline bool needsSingleQuotes(StringRef S) {
   if (S.empty())
     return true;
   if (isspace(S.front()) || isspace(S.back()))
     return true;
   if (S.front() == ',')
     return true;
----------------
Can't you combine the two functions to a single one so we only need to scan the string once? (Just keep the checks at the begin and end of needsSingleQuote() around and then perform the scan from needsDoubleQuotes() while additionally checking the things from ScalarSafeChars. Instead of returning false/true you would compute the max required quoting)


================
Comment at: lib/Support/YAMLTraits.cpp:644-647
+    if (S[j] == QuoteChar) {
       output(StringRef(&Base[i], j - i + 1));
-      output("'");
+      output(Quote);
       i = j + 1;
----------------
While I guess it's not strictly necessary, how about something along the lines of:
```
// Note: Special casing the non-printable ASCII characters.
if (S[j] == QuoteChar || (MustQuote == QuotingType::Double && S[j] <= 0x1F)) {
  output(...)
  if (S[j] <= 0x1f) {
    output("\\0x" + hex(S[j]));
  } else {
    output(Quote);
  }
  i = j+1;
}
```

so we get a nice `\0x01` output instead of a quoted unprintable character?


https://reviews.llvm.org/D41290