[PATCH] D33961: encode YAML scalars using double quotes so all values can be expressed

[Bob Haarman via Phabricator via llvm-commits]

inglorion added inline comments.


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:623
+    char C = Base[i];
+    if (C < 32 || C > 126) {
+      static const char hexits[17] = "0123456789abcdef";
----------------
zturner wrote:
> Is there any reason not to use `isprint` here (the semantics of `isprint` are slightly different than your if-case, btw)
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?


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:624-627
+      static const char hexits[17] = "0123456789abcdef";
+      Encoded += "\\x";
+      Encoded += hexits[(C >> 4) & 0xf];
+      Encoded += hexits[C & 0xf];
----------------
zturner wrote:
> You could use `Encoded += "\\x" + toHex(StringRef(&C, 1));` here.
Will do.


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:629
+    } else {
+      if (C == '\"' || C == '\\') {
+        Encoded += '\\';
----------------
zturner wrote:
> 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.
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).


https://reviews.llvm.org/D33961