Hi Dodji, I appreciate your review but I'm replying for now only for the
libiberty.h part (attached updated patch), which I've needed elsewhere and
seems the easiest to quickly apply.
On Wed, 18 Jul 2012, Dodji Seketeli wrote:
=== modified file 'include/libiberty.h'
--- include/libiberty.h 2011-09-28 19:04:30 +0000
+++ include/libiberty.h 2012-07-07 16:04:01 +0000
[...]
-/* Type-safe obstack allocator. */
+/* Type-safe obstack allocator. You must first initialize the obstack with
+ obstack_init() or _obstack_begin(). */
Why not just using the _obstack_begin function? Why introducing the
use of the obstack_init macro?
Please correct me if I'm wrong, but my impression is that obstack_init
is the documented way (see [1]), _obstack_begin seems hidden and gives
access to dangerous (performance-wise) parameters, even though we always
use it with the defaults (hmmm double checking I see that we mostly
set size 0 which is default, but also alignment 0???).
[1]
http://www.gnu.org/software/libc/manual/html_node/Preparing-for-Obstacks.html
-#define XOBFINISH(O, T) ((T) obstack_finish ((O)))
I think this change is not needed. You basically remove this line to
replace it with the hunk below, that comes later in the patch:
+#define XOBFINISH(O, PT) ((PT) obstack_finish ((O)))
So I believe you could do away with the change.
I just wanted to point out that it expects and returns a Pointer to the
Type, e.g. after you XOBGROW(O, int, DATA) you should XOBFINISH(O, int *).
It would probably be better for XOBFINISH to accept T and return (T *),
but I didn't want to change existing status quo.
I think I addressed your other concerns (style, comments, unneeded macro
params). CC'd maintainers. What do you think about attached patch?
Thanks,
Dimitris
P.S. Personally I prefer the original obstack interface, i.e.
obstack_grow(), obstack_finish() etc. I just added the macros to follow
existing style and avoid typecasting because of C++. Let me know if it's
OK to use obstacks directly and I'll back out the patch.
2012-08-04 Dimitrios Apostolou <ji...@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.
=== 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 */
