Re: [PATCH 4/4] gdb: make internalvar use a variant

[Pedro Alves <pedro@palves.net>]

On 2022-03-04 16:23, Tom Tromey wrote:
> Simon> The advantage of a variant here is that we can more easily use non-POD
> Simon> types in the alternatives.  And since it remembers which alternative is
> Simon> the current one, accesses are checked so that accesses to the wrong
> Simon> alternative throw an exception (see question about that below).
> 
> I was wondering if we really need internalvars that can "switch" like
> this.  Like, could we just subclass internalvar instead and get rid of
> the "kind" / payload stuff.
> 
> [...]
> Simon> with some functor + std::visit.
> 
> FWIW I tend to dislike the visitor pattern anyhow.  IME it is just a
> pain to use well -- lots of callbacks, the code becomes unreadable.

+1, my thoughts exactly.

It requires more C++ expertise than most of other things to know to write such code.  Something that should be
simple is complicated.  C++ is really lacking proper pattern matching and builtin variant types, to make
variants easy to use.  But if we use it in a contained way like basically glorified tagged union to get rid
of some boilerplace we have to write today,  I guess it's OK.


Even if we use a variant in the internalvar case, I'd think just assuming that enum values
have the same order as the variant types would be so much easier to follow and code.

I mean, something like this:

  /* Return the kind of the internalvar.  */
  internalvar_kind kind () const
  { return (enum internalvar_kind) v.index (); }

Instead of the currently proposed:

  /* Return the kind of the internalvar.  */
  internalvar_kind kind () const
  {
    struct visitor
    {
      internalvar_kind operator() (const internalvar_void &void_) const
      { return INTERNALVAR_VOID; }

      internalvar_kind operator() (const internalvar_value &value) const
      { return INTERNALVAR_VALUE; }

      internalvar_kind operator() (const internalvar_make_value &make_value) const
      { return INTERNALVAR_MAKE_VALUE; }

      internalvar_kind operator() (const internalvar_function &void_) const
      { return INTERNALVAR_FUNCTION; }

      internalvar_kind operator() (const internalvar_integer &integer) const
      { return INTERNALVAR_INTEGER; }
 
      internalvar_kind operator() (const internalvar_string &string) const
      { return INTERNALVAR_STRING; }
    };

    return nonstd::visit (visitor {}, this->v);
  }


How debuggeable is nonstd::variant?  When printing a tagged union in GDB, it's easy to figure out the
union's current value.  What does printing a nonstd::variant (not mapped to the std variant) look
like?  AFAICT from variant.cpp source file, the variant storage is an untyped buffer:

    enum { data_align = detail::typelist_max_alignof< variant_types >::value };

    using aligned_storage_t = typename std::aligned_storage< data_size, data_align >::type;
    aligned_storage_t data;

so I guess printing a nonstd::variant results in pretty opaque output.  We'd need a pretty
printer to fix this.  Or maybe we just assume that people developing/debugging GDB build
it against a C++17 or higher compiler?  (Not sure that's a great assumption.)


I wonder whether we should do:

 namespace gdb {
   using namespace nonstd;
 }

and then use gdb::variant throughout, instead of "nonstd::variant".  At least "gdb"
is three letters, and thus less code/indentation churn when we someday switch to "std".
Also less churn if we move to some other variant type, though not sure that's likely
after we start using one.