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

[Zachary Turner via llvm-commits]

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
On Wed, Jun 7, 2017 at 4:49 PM Bob Haarman via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170608/238592d9/attachment.html>