This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] Class-ify ptid_t


On 17-04-07 05:25 AM, Philipp Rudo wrote:
> Hi Simon,
> 
> I know I'm a little late, but there are some inconsistencies/ambiguities
> in the error messages of the unittests bothering me.  For example

Hi Philipp, thanks for looking at this!

>> +static_assert (!pid.matches (lwp), "pid matches lwp");
>> +static_assert (lwp.matches (lwp), "lwp matches lwp");
> [...]
>> +static_assert (!ptid_t (2, 2, 0).matches (lwp), "other lwp doesn't
>> match lwp");
> 
> where the 1st and 2nd asserts have the same error message different to
> the 3rd one.  Although the 1st and 3rd assert the same (not matching)
> while the 2nd asserts the opposite (matching). 

Oops,

  static_assert (!pid.matches (lwp), "pid matches lwp");

is definitely a copy-paste leftover.  It should've said "pid doesn't match lwp".  There
are a few instances of that.

> Or
> 
>> +static_assert (pid == ptid_t (1, 0, 0), "pid operator== is right")
> 
> where the only time you see the error message is when operator==
> returns false.  But in that case operator== isn't right but wrong ;)
> 
> In this context I ask myself what the error message is supposed to
> say (unfortunately you are the first one in GDB actually using it, all
> others only use an empty string).  Is it what's expected ...
> 
> static_assert (!pid.matches (lwp), "pid mustn't match lwp");
> static_assert (lwp.matches (lwp), "lwp must match lwp/itself");
> 
> static_assert (pid == ptid_t (1, 0, 0), "pid operator== must be true")
> 
> 
> ... or what went wrong?
> 
> static_assert (!pid.matches (lwp), "pid matches lwp");
> static_assert (lwp.matches (lwp), "lwp doesn't match lwp/itself");
> 
> static_assert (pid == ptid_t (1, 0, 0), "pid operator== returned false")
> 
> 
> I tend more to what is expected.

I agree, that way it matches the condition right next to it.  But the verb tense is
probably not the best.  must/mustn't or should/shouldn't may be clearer than does/doesn't.

I'll at least fix what's obviously wrong right now, if you want to take a shot
at improving the messages, you are welcome :).

> Otherwise your series is great. This will definitely help making the
> code better readable.
> 
> Thanks
> Philipp

Thanks,

Simon


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]