[PATCH] D79745: [YAMLTraits] Add trait for char

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

In D79745#2050040 <https://reviews.llvm.org/D79745#2050040>, @ro wrote:

> In D79745#2049359 <https://reviews.llvm.org/D79745#2049359>, @JDevlieghere wrote:
>
> > In D79745#2049287 <https://reviews.llvm.org/D79745#2049287>, @ro wrote:
> >
> > > In D79745#2049234 <https://reviews.llvm.org/D79745#2049234>, @JDevlieghere wrote:
> > >
> > > >
> > >
> >
>
>
>
>
> >> I've checked the gcc configs and of those, only on Solaris and BPF is `int8_t` plain `char`.  Elsewhere they use explicit `signed char` or
> >>  `unsigned char`.  Haven't checked what the C standard requires/allows here, but AFAIK Sun had a couple of guys on the C committee at
> >>  the time, so they should know what they're doing...
> > 
> > The C++ standard [1] says that `char`, `unsigned char` and `signed char` are distinctive types. However, I doesn't specify how `uint8_t` is implemented. I think maybe the right thing to do is wrap the YAML trait in a ifdef that excludes Solaris and BPF. What do you think?
> > 
> > [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf ([basic.fundamental])
>
> TBH, feels like a hack to me: if `int8_t` can be either `char` (if signed) or `signed char`, the code should deal with both possibilities.


It just seems like such a weird decision, if you must have to have 3 distinct types according to the standard, why would you choose `char` (for which it's implementation defined whether it can be negative or not) instead of `unsigned char` (which is guaranteed to be unsigned). Maybe I'm missing something?

> That said, it would certainly unbreak the Solaris builds which for now is my primary concern.  AFAIK there's no lldb port to Solaris, though
>  I vaguely recall that there was an attempt in the Illumos (OpenSolaris derivative) community.  On top of that, those static lists are quite
>  fragile IMO: something like this should be feature-based instead, but that is an all too common problem all over LLVM.
> 
> What about your idea of simply casting to `int8_t` in lldb instead?

Given that the types are distinct this would require a `reinterpret_cast` which feels more hacky than ifdef'ing the trait for the "odd" choice in solaris.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79745