Re: [PATCH 2/2] Class-ify ptid_t

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-04-05 17:31, Pedro Alves wrote:
> -  bool is_null () const
> +  constexpr bool is_null () const
>    {
>      return *this == null_ptid;
>    }
>
> -  bool is_any () const
> +  constexpr bool is_any () const
>    {
>      return *this == minus_one_ptid;
>    }

Hmm I think there's a problem with these.  null_ptid and minus_one_ptid 
are not constant expressions, so these methods (and the ones that use 
them) are not really constexpr (you notice when you actually try to use 
them as constant expressions).  Making null_ptid and minus_one_ptid 
constexpr would be useful, I could use them in the static_assert tests 
(e.g. to test "matches").  However, when trying to make them constexpr, 
it creates a dependency loop: the class needs to be defined after the 
null_ptid/minus_one_ptid definitions because it uses them (and you can't 
forward declare a constexpr variable I suppose?), but the 
null_ptid/minus_one_ptid definitions have to be be defined after the 
class, when the type is complete.

An easy workaround would be to not refer to null_ptid/minus_one_ptid in 
the class, but use manually crafted values:

  constexpr bool is_null () const
  {
    return *this == ptid_t (0, 0, 0);
  }

  constexpr bool is_any () const
  {
    return *this == ptid_t (-1, 0, 0);
  }

And to avoid duplication, use defines:

/* Work around the fact that we want to refer to null_ptid and 
minus_one_ptid
   in the definition of class ptid_t, but they have to be defined after. 
 */
#define NULL_PTID ptid (0, 0, 0)
#define MINUS_ONE_PTID ptid (-1, 0, 0)

class ptid_t
{
  ...
  constexpr bool is_null () const
  {
    return *this == NULL_PTID;
  }

  constexpr bool is_any () const
  {
    return *this == MINUS_ONE_PTID;
  }
  ...
}

constexpr ptid_t null_ptid = NULL_PTID;
constexpr ptid_t minus_one_ptid = MINUS_ONE_PTID;

/* We don't want anybody using these macros, they are just temporary.
#undef NULL_PTID
#undef MINUS_ONE_PTID

What do you think?

Now, making null_ptid/minus_one_ptid constexpr brings its share of 
fallouts, such as:

/home/simark/src/binutils-gdb/gdb/linux-nat.c: In function ‘void 
linux_unstop_all_lwps()’:
/home/simark/src/binutils-gdb/gdb/linux-nat.c:2387:37: error: invalid 
conversion from ‘const void*’ to ‘void*’ [-fpermissive]
        resume_stopped_resumed_lwps, &minus_one_ptid);
                                     ^~~~~~~~~~~~~~~
/home/simark/src/binutils-gdb/gdb/linux-nat.c:980:1: note:   
initializing argument 3 of ‘lwp_info* iterate_over_lwps(ptid_t, int 
(*)(lwp_info*, void*), void*)’
 iterate_over_lwps (ptid_t filter,
 ^~~~~~~~~~~~~~~~~

But it looks easy enough to fix by C++-ifying/modernizing 
iterate_over_lwps.