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

[Rainer Orth via Phabricator via llvm-commits]

ro added a comment.

>>> 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?

I've done some digging: `<sys/int_types.h>` has

  /*
   * Basic / Extended integer types
   *
   * The following defines the basic fixed-size integer types.
   *
   * Implementations are free to typedef them to Standard C integer types or
   * extensions that they support. If an implementation does not support one
   * of the particular integer data types below, then it should not define the
   * typedefs and macros corresponding to that data type.  Note that int8_t
   * is not defined in -Xs mode on ISAs for which the ABI specifies "char"
   * as an unsigned entity because there is no way to define an eight bit
   * signed integral.
   */
  #if defined(_CHAR_IS_SIGNED)
  typedef char                    int8_t;
  #else
  #if defined(__STDC__)
  typedef signed char             int8_t;
  #endif
  #endif

Both sparc and x86 define `_CHAR_IS_SIGNED` in accordance with the respective psABIs, and when those definitions were made,
Solaris needed to support both K&C and ANSI C.  Once the definition was made this way, it had to stick, otherwise e.g. C++ mangling
changes.

That's what you get on a system that has been around for almost 3 decades :-)

>> 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.

However, it would work everywhere instead of leaving systems with `char` for `int8_t` in the cold.

Your choice, of course.

Btw., I happened to notice some formatting inconsistencies in your patch that you may want to correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79745