[asterisk-dev] [Code Review] remove checks for alloca and ast_strdupa NULL return value

[Simon Perreault]

On 07/16/2012 08:36 AM, Walter Doekes wrote:
> On 12/07/12 21:00, Simon Perreault wrote:
>>> But, I believe the gcc __builtin_alloca guarantees it won't return NULL,
>>> but fail in more mysterious ways instead or SEGFAULT. So, I added an
>>> ast_alloca macro that expands to that.
>>
>> What do you think alloca() expands to on FreeBSD?
>>
>> That's right! __builtin_alloca()!
>
> I wouldn't know. I don't have any BSD boxes.

So maybe you should investigate a little bit before proposing this 
patch? Spend an hour making yourself a FreeBSD VM and looking at things?

> So, unless someone says that __builtin_alloca() can return NULL on
> FreeBSD, then both patch (1) and patch (2) should be OK.

I have no idea. Sorry to be blunt, but you should be doing your 
homework, not relying on reviewers to gather information for you.

> I'd still prefer patch (2), since I read that gcc uses __builtin_alloca
> only for -O2 or higher -- we might get a different implementation for
> non-optimized builds on certain systems(?).
>
> In addition, the ast_strdupa macro also refers to __builtin_alloca()
> directly instead of to alloca(). It then makes sense to not use alloca()
> in the rest of the source either.

You say GCC uses __builtin_alloca() for -O2 only. Your patch overrides 
this behaviour and makes __builtin_alloca() used at all levels. This 
seems wrong to me.

It seems to me like you're taking a stab in the dark, not knowing 
exactly what you should be doing. Unless you can come up with hard 
facts, and a good reason to change things, I would suggest not touching 
anything.

Simon
-- 
DTN made easy, lean, and smart --> http://postellation.viagenie.ca
NAT64/DNS64 open-source        --> http://ecdysis.viagenie.ca
STUN/TURN server               --> http://numb.viagenie.ca