This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Class-ify ptid_t
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Philipp Rudo <prudo at linux dot vnet dot ibm dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Fri, 7 Apr 2017 10:25:58 -0400
- Subject: Re: [PATCH v2] Class-ify ptid_t
- Authentication-results: sourceware.org; auth=none
- Authentication-results: sourceware.org; dkim=none (message not signed) header.d=none;sourceware.org; dmarc=none action=none header.from=ericsson.com;
- References: <20170406190328.21103-1-simon.marchi@ericsson.com> <20170407112528.3f03fd37@ThinkPad>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
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