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

[Pedro Alves <palves@redhat.com>]

On 04/06/2017 03:15 AM, Simon Marchi wrote:
> On 2017-04-05 17:31, Pedro Alves wrote:
>>>> It's probably going to be worth it to sprinkle "constexpr"
>>>> all over the new API.  Helps with static_asserts in
>>>> unit testing too.  *cough*  :-)
>>>
>>> Ok, will look into it.
>>
>> Thanks.  constexpr in C++11 forces you to write a single
>> return statement (C++14 gets rid of that requirement),
>> but it looks quite doable.
>>
>> Also, note that it's not true that this type can't have a
>> constructor.  It can, as long as the type remains POD.
> 
> Ah, so I was just missing the defaulted default constructor.  Adding it
> makes the type trivial, which then makes it POD.

Right, almost.  There's a couple other requirements beyond trivial,
but they're fulfilled as well.  See:

 http://en.cppreference.com/w/cpp/concept/PODType

> 
>>> Is the following ok?
>>>
>>>   struct thread_resume default_action { null_ptid };
>>
>> ISTR that in C++11 you'll need the double "{{" levels, like:
>>
>>    thread_resume default_action {{null_ptid}};
>>
>> and that C++14 removed that requirement (brace elision).
>> But I haven't confirmed.  Whatever works with -std=c++11.
> 
> It seems to work with a single pair of braces with c++11.  I'll still
> check that it does what we want at runtime, but I'd be surprised if it
> didn't do the right thing.

Sorry, yes, it's not necessary -- I somehow confused myself into 
thinking that the current double "{{" was because the ptid field was 
inside a structure that itself is inside the thread_resume structure.

struct thread_resume
{
   struct something_else 
   {
      ptid_t thread;


but "something_else" doesn't really exist...  If it existed, then
the double {{ would be necessary in C++11 to initialize the sub field, but
not in C++.   See brace elision here:
http://en.cppreference.com/w/cpp/language/aggregate_initialization

But now that I try, I can't make g++ nor clang++ error in c++11 with
those constructs.  Maybe other compilers would, though.  In any
case, the workaround would be trivial if we needed it -- just
add the equals sign.

>>
>>
>> +static_assert (std::is_pod<ptid_t>::value, "");
>> +
>> +static_assert (ptid_t(1).pid () == 1, "");
> 
> Wow, nice.  So all the tests are probably going to be static.
> 
> Just to be clear, do you suggest that we make a test that ensures ptid_t
> is a POD, or you wrote that one just to show me it works?  I We don't
> really care if it is, it's just that the current situation (it being
> used in another POD) requires it.

Yes, I think we should put that in the unit test with a comment
so that if someone tries to add something that would make it
non-pod, gdb won't even compile.  If/when we get to the point where
we can/want to make it non-pod, we can remove the assertion then.

Thanks,
Pedro Alves