Message ID | alpine.LNX.2.02.1207312009120.20463@localhost.localdomain |
---|---|
State | New |
Headers | show |
> 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." > + 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. > +#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 *)? Ian
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?). > >> + 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. > >> +#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. Thanks, Dimitris
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
On Sat, 4 Aug 2012, Ian Lance Taylor wrote: >> On Fri, 3 Aug 2012, Ian Lance Taylor wrote: > > 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. In libcpp/ mostly, but only 4 cases so that's not too many, I can change those with obstack_init/begin. >>>> +#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. No problem, if anyone else doesn't object I'll change those (in a second patch, right?). Thanks, Dimitris
On Sun, Aug 5, 2012 at 11:44 AM, Dimitrios Apostolou <jimis@gmx.net> wrote: > > No problem, if anyone else doesn't object I'll change those (in a second > patch, right?). Can you please resend the patch you propose to check into mainline? Thanks. Ian
=== modified file 'include/libiberty.h' --- include/libiberty.h 2011-09-28 19:04:30 +0000 +++ include/libiberty.h 2012-08-03 23:51:55 +0000 @@ -361,12 +361,21 @@ extern unsigned int xcrc32 (const unsign #define XDUPVAR(T, P, S1, S2) ((T *) xmemdup ((P), (S1), (S2))) #define XRESIZEVAR(T, P, S) ((T *) xrealloc ((P), (S))) -/* Type-safe obstack allocator. */ +/* Type-safe obstack allocator. You must first initialize the obstack with + obstack_init() or _obstack_begin(). + T: Type, O: Obstack, N: Number of elements, S: raw Size, + PT: Pointer to T, D: Data, P: Pointer to element. */ #define XOBNEW(O, T) ((T *) obstack_alloc ((O), sizeof (T))) #define XOBNEWVEC(O, T, N) ((T *) obstack_alloc ((O), sizeof (T) * (N))) #define XOBNEWVAR(O, T, S) ((T *) obstack_alloc ((O), (S))) -#define XOBFINISH(O, T) ((T) obstack_finish ((O))) +#define XOBDELETE(O, P) (obstack_free ((O), (P))) + +#define XOBGROW(O, T, D) obstack_grow ((O), (D), sizeof (T)) +#define XOBGROWVEC(O, T, D, N) obstack_grow ((O), (D), sizeof (T) * (N)) +#define XOBSHRINK(O, T) obstack_blank ((O), -1 * sizeof (T)) +#define XOBSHRINKVEC(O, T, N) obstack_blank ((O), -1 * sizeof (T) * (N)) +#define XOBFINISH(O, PT) ((PT) obstack_finish ((O))) /* hex character manipulation routines */