Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete

[Pedro Alves <palves at redhat dot com>]

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