Richard W.M. Jones
2023-Feb-22 17:01 UTC
[Libguestfs] [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY
On Wed, Feb 22, 2023 at 05:22:57PM +0100, Laszlo Ersek wrote:> On 2/22/23 10:40, Laszlo Ersek wrote: > > On 2/22/23 09:17, Richard W.M. Jones wrote: > >> On Tue, Feb 21, 2023 at 05:23:52PM +0100, Laszlo Ersek wrote: > > > >>> This is doable, but I hope it's not expected that > >>> DEFINE_POINTER_VECTOR_TYPE() *enforce* that the element type be a pointer :) > >> > >> > >> You might ignore this for a first draft, but it is apparently possible > >> to statically detect this (at least, if using GCC/clang): > >> > >> https://stackoverflow.com/questions/19255148/check-if-a-macro-argument-is-a-pointer-or-not > > > > Right, we already use at least __builtin_types_compatible_p in > > TYPE_IS_ARRAY(); that's what I wouldn't want more of, at least via this > > series. > > OK, summarizing the TODOs for this particular patch: > > 1. keep _iter > > 2. rename DEFINE_VECTOR_EMPTY to ADD_VECTOR_EMPTY_METHOD > > 3. introduce DEFINE_POINTER_VECTOR_TYPE that expands to DEFINE_VECTOR_TYPE + ADD_VECTOR_EMPTY_METHOD > > 4. DEFINE_POINTER_VECTOR_TYPE (and ADD_VECTOR_EMPTY_METHOD) should not take "free" as a parameter; ADD_VECTOR_EMPTY_METHOD should hard-code it > > 5. in "common/utils/string-vector.h", don't ADD_VECTOR_EMPTY_METHOD; instead, replace DEFINE_VECTOR_TYPE with DEFINE_POINTER_VECTOR_TYPE. > > 6. in ADD_VECTOR_EMPTY_METHOD, consider checking that the element type is a pointer type.Sounds like a plan.> --*-- > > Re: 6, I find that the stackoverflow solution above is too complicated. I mentioned our existent TYPE_IS_ARRAY macro: > > #define TYPE_IS_ARRAY(a) \ > (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) > > This is perfectly usable for our purposes, as !TYPE_IS_ARRAY(). The reason is that when this macro is applied to anything that's *neither* a pointer *nor* an array, we get a build error at once: > > #define TYPE_IS_ARRAY(a) \ > (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) > > int main(void) > { > int x[5]; > int *y; > int z; > > TYPE_IS_ARRAY (x); > TYPE_IS_ARRAY (y); > TYPE_IS_ARRAY (z); > return 0; > } > > ---> > > isptr.c: In function ?main?: > isptr.c:2:59: error: subscripted value is neither array nor pointer nor vector > 2 | (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) > | ^ > isptr.c:12:3: note: in expansion of macro ?TYPE_IS_ARRAY? > 12 | TYPE_IS_ARRAY (z); > | ^~~~~~~~~~~~~ > > (the "nor vector" part of the error message can most likely be ignored, I believe it refers to the C++ standard library vector class) > > Thus, with types that are neither pointers nor arrays nicely caught at compilation time, !TYPE_IS_ARRAY stands for "pointer".Nice .. but probably want to define: #define TYPE_IS_POINTER(p) (!TYPE_IS_ARRAY(p))> I'll experiment with this a bit, but if it becomes too complex, I'll likely drop step 6.Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Laszlo Ersek
2023-Feb-23 17:54 UTC
[Libguestfs] [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY
On 2/22/23 18:01, Richard W.M. Jones wrote:> On Wed, Feb 22, 2023 at 05:22:57PM +0100, Laszlo Ersek wrote: >> On 2/22/23 10:40, Laszlo Ersek wrote: >>> On 2/22/23 09:17, Richard W.M. Jones wrote: >>>> On Tue, Feb 21, 2023 at 05:23:52PM +0100, Laszlo Ersek wrote: >>> >>>>> This is doable, but I hope it's not expected that >>>>> DEFINE_POINTER_VECTOR_TYPE() *enforce* that the element type be a pointer :) >>>> >>>> >>>> You might ignore this for a first draft, but it is apparently possible >>>> to statically detect this (at least, if using GCC/clang): >>>> >>>> https://stackoverflow.com/questions/19255148/check-if-a-macro-argument-is-a-pointer-or-not >>> >>> Right, we already use at least __builtin_types_compatible_p in >>> TYPE_IS_ARRAY(); that's what I wouldn't want more of, at least via this >>> series. >> >> OK, summarizing the TODOs for this particular patch: >> >> 1. keep _iter >> >> 2. rename DEFINE_VECTOR_EMPTY to ADD_VECTOR_EMPTY_METHOD >> >> 3. introduce DEFINE_POINTER_VECTOR_TYPE that expands to DEFINE_VECTOR_TYPE + ADD_VECTOR_EMPTY_METHOD >> >> 4. DEFINE_POINTER_VECTOR_TYPE (and ADD_VECTOR_EMPTY_METHOD) should not take "free" as a parameter; ADD_VECTOR_EMPTY_METHOD should hard-code it >> >> 5. in "common/utils/string-vector.h", don't ADD_VECTOR_EMPTY_METHOD; instead, replace DEFINE_VECTOR_TYPE with DEFINE_POINTER_VECTOR_TYPE. >> >> 6. in ADD_VECTOR_EMPTY_METHOD, consider checking that the element type is a pointer type. > > Sounds like a plan. > >> --*-- >> >> Re: 6, I find that the stackoverflow solution above is too complicated. I mentioned our existent TYPE_IS_ARRAY macro: >> >> #define TYPE_IS_ARRAY(a) \ >> (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) >> >> This is perfectly usable for our purposes, as !TYPE_IS_ARRAY(). The reason is that when this macro is applied to anything that's *neither* a pointer *nor* an array, we get a build error at once: >> >> #define TYPE_IS_ARRAY(a) \ >> (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) >> >> int main(void) >> { >> int x[5]; >> int *y; >> int z; >> >> TYPE_IS_ARRAY (x); >> TYPE_IS_ARRAY (y); >> TYPE_IS_ARRAY (z); >> return 0; >> } >> >> ---> >> >> isptr.c: In function ?main?: >> isptr.c:2:59: error: subscripted value is neither array nor pointer nor vector >> 2 | (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) >> | ^ >> isptr.c:12:3: note: in expansion of macro ?TYPE_IS_ARRAY? >> 12 | TYPE_IS_ARRAY (z); >> | ^~~~~~~~~~~~~ >> >> (the "nor vector" part of the error message can most likely be ignored, I believe it refers to the C++ standard library vector class) >> >> Thus, with types that are neither pointers nor arrays nicely caught at compilation time, !TYPE_IS_ARRAY stands for "pointer". > > Nice .. but probably want to define: > > #define TYPE_IS_POINTER(p) (!TYPE_IS_ARRAY(p)) > >> I'll experiment with this a bit, but if it becomes too complex, I'll likely drop step 6.Sigh, this patch has now turned into a series of 5 patches. I'll post it separately tomorrow. /smh Laszlo