Re: [libiberty] add obstack macros (was Re: PR #53525 - track-macro-expansion performance regression)

[Ian Lance Taylor <iant at google dot com>]

On Sat, Aug 4, 2012 at 9:40 AM, Dimitrios Apostolou <jimis@gmx.net> wrote:
> On Fri, 3 Aug 2012, Ian Lance Taylor wrote:
>
>>> 2012-08-04 Dimitrios Apostolou <jimis@gmx.net>
>>>
>>>         * libiberty.h
>>>         (XOBDELETE,XOBGROW,XOBGROWVEC,XOBSHRINK,XOBSHRINKVEC): New
>>>         type-safe macros for obstack allocation.
>>>         (XOBFINISH): Renamed argument to PT since it is a pointer to T.
>>
>>
>>> +/* Type-safe obstack allocator. You must first initialize the obstack
>>> with
>>> +   obstack_init() or _obstack_begin().
>>
>>
>> This should recommend obstack_init, or obstack_begin, but not
>> _obstack_begin.  Also obstack_specify_allocation and
>> obstack_specify_allocation_with_arg are OK, so really it might be
>> better not to list the functions, but simply say "You must first
>> initialization the obstack."
>
>
> Grep reveals that 9 out of 10 times we use _obstack_begin(), and we set
> alignment to 0 (isn't it suboptimal?).

I'm not sure where you are looking.  I only see one call to
_obstack_begin in the gcc directory, and it could easily be replaced
with a call to obstack_specify_allocation instead.  It does set the
alignment to 0, but that just directs the obstack code to use the
default alignment, which is the alignment of double.  I think that
should normally be fine.


>>> +   T: Type,  O: Obstack,  N: Number of elements,  S: raw Size,
>>
>>
>> s/Size/size/
>>
>>> +#define XOBSHRINK(O, T)                obstack_blank ((O), -1 * sizeof
>>> (T))
>>> +#define XOBSHRINKVEC(O, T, N)  obstack_blank ((O), -1 * sizeof (T) *
>>> (N))
>>
>>
>> These are hard to use safely.  I'm not sure we should define them at all.
>
>
> I've already used XOBSHRINK and it looks clear to me, but I could use
> obstack_blank() directly if necessary.

I'm a bit concerned because they only work if space has already been
allocated, and there is no checking.  Also I only see them used in
genautomata.c.  But I guess it's OK.


>>> +#define XOBFINISH(O, PT)       ((PT) obstack_finish ((O)))
>>
>>
>> For XOBNEW, etc., we use (T *) rather than (PT).  Using (PT) seems
>> error-probe--it's the only use of the obstack with a different type
>> parameter.  Why not use T rather than PT here, and return (T *)?
>
>
> I'd have to change many (about 60) occurences of XOBFINISH if I change that.
> I'd go for it if I was sure it's what we want, it can be a separate patch
> later on.

I'm sorry to ask you to change a lot of code, but it simply doesn't
make sense to me to have all but one macro take the type as an
argument, and have one macro take a pointer to the type.  They really
have to be consistent.

Ian