Yea let's use is print.  I had to go lookup where the magic values 32 and 127 came from, so at least with isprint the code is immediately understandable why we're doing this<br><div class="gmail_quote"><div dir="ltr">On Wed, Jun 7, 2017 at 4:49 PM Bob Haarman via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">inglorion added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/lib/Support/YAMLTraits.cpp:623<br>
+    char C = Base[i];<br>
+    if (C < 32 || C > 126) {<br>
+      static const char hexits[17] = "0123456789abcdef";<br>
----------------<br>
zturner wrote:<br>
> Is there any reason not to use `isprint` here (the semantics of `isprint` are slightly different than your if-case, btw)<br>
Not really. I just don't know off the top of my head if isprint checks the same thing as YAML's "printable character". For characters with code points >=0 and <= 127, it seems like it does. Since I'm not handling Unicode here, I could presumably replace the check here with isprint. Would you like me to do that?<br>
<br>
<br>
================<br>
Comment at: llvm/lib/Support/YAMLTraits.cpp:624-627<br>
+      static const char hexits[17] = "0123456789abcdef";<br>
+      Encoded += "\\x";<br>
+      Encoded += hexits[(C >> 4) & 0xf];<br>
+      Encoded += hexits[C & 0xf];<br>
----------------<br>
zturner wrote:<br>
> You could use `Encoded += "\\x" + toHex(StringRef(&C, 1));` here.<br>
Will do.<br>
<br>
<br>
================<br>
Comment at: llvm/lib/Support/YAMLTraits.cpp:629<br>
+    } else {<br>
+      if (C == '\"' || C == '\\') {<br>
+        Encoded += '\\';<br>
----------------<br>
zturner wrote:<br>
> It looks like the previous code did not escape backslashes.  Was this a bug in the previous code?  If so we should add a test for it.<br>
The old code didn't escape backslashes, which is actually correct - single quotes in YAML don't support any escape sequences, with the exception of two single quotes to encode a single single quote. Backslashes are used in some of the tests, so we know they are handled correctly both before and after this change, but I'll add an explicit test in YAMLIOTest (where I've already added tests for control characters and characters we expect to appear literally).<br>
<br>
<br>
<a href="https://reviews.llvm.org/D33961" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33961</a><br>
<br>
<br>
<br>
</blockquote></div>