This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete
On 11/23/2017 05:27 PM, Simon Marchi wrote:
> On 2017-11-23 10:02, Pedro Alves wrote:
>> On 11/22/2017 04:41 PM, Simon Marchi wrote:
>>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> The criterion I
>>> initially used to allow a type to use XNEW (which calls malloc in the
>>> end) was std::is_trivially_constructible, but then realized that gcc 4.8
>>> did not have it. Instead, I went with:
>>>
>>
>> Yeah, in the memset poisoning I just disabled the poisoning on older
>> compilers. This is the "#if HAVE_IS_TRIVIALLY_COPYABLE" check in
>> poison.h.
>
> I'm not sure if you are suggesting adding
> HAVE_IS_TRIVIALLY_CONSTRUCTIBLE, but since we can use std::is_pod
> easily, I think it's simpler just to use that (even if it's a bit more
> strict than needed).
Nope, I was really just stating a fact.
>>> which is just a bit more strict, which doesn't hurt. A similar thing is
>>> done for macros that free instead of allocated, the criterion is:
>>>
>>> using IsFreeable = gdb::Or<std::is_trivially_destructible<T>,
>>> std::is_void<T>>;
>>>
>>> For simplicity, we could also do for std::is_pod for IsFreeable as well,
>>> if you prefer.
>>>
>>> I chose to put static_assert in the functions, instead of using
>>> gdb::Requires in the template as SFINAE, because it allows to put a
>>> message, which I think makes the compiler error more understandable.
>>> With gdb::Requires, the error is:
>>
>> Right, I think static_assert is what I had suggest initially too.
>> SFINAE alone wouldn't sound right to me here.
>>
>> The right alternative to static_assert would be SFINAE+"=delete;",
>> which is what we do for the memcpy/memcpy poisoning, and what
>> you're doing to "free".
>>
>> =delete keeps the function/overload in the overload set, serving
>> as "honeypot", so you get an error about trying to call a
>> deleted function.
>>
>> You could do that to xfree instead
>> of the static_assert, but I guess inlining xfree ends up being
>> a good thing.
>
> Ah yeah, it would probably make the message a bit clearer. Instead of
> saying "I can't find a function that matches", it would say "I found a
> function that matches, but you can't use it".
Exactly.
> I'll just keep
> static_assert for xfree as well, since it ends up clearer anyway.
*nod*
>> Other than the formatting, this LGTM. Thanks a lot for doing this.
>
> Thanks, I'm glad that I won't have to carry that branch on the side
> anymore :)
>
:-)
Thanks,
Pedro Alves