Re: [Libguestfs] [PATCH libnbd v3 2/6] generator: Add attribute((nonnull)) annotations to non-NULL parameters

On Wed, Sep 28, 2022 at 06:25:35PM +0100, Richard W.M. Jones wrote: > For API parameters that are pointers and must not be NULL, add the > appropriate GCC annotations. > > Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > generator/C.ml | 59 +++++++++++++++++++++++++++++++++++-- > tests/errors-connect-null.c | 4 +++ > 2 files changed, 61 insertions(+), 2 deletions(-) > @@ -216,7 +238,21 @@ let > let print_fndecl ?wrap ?closure_style name args optargs ret = > pr "extern "; > print_call ?wrap ?closure_style name args optargs ret; > - pr ";\n" > + > + (* Output __attribute__((nonnull)) for the function parameters: > + * eg. struct nbd_handle *, int, char * > + * => [ true, false, true ] > + * => LIBNBD_ATTRIBUTE_NONNULL((1,3)) > + * => __attribute__((nonnull,(1,3))) Style question. Do we want to REQUIRE the clients of this macro to pass in (), or would it be better to have a varargs format? > + *) > + let nns : bool list = > + [ true ] (* struct nbd_handle * *) > + @ List.flatten (List.map arg_attr_nonnull args) > + @ List.flatten (List.map optarg_attr_nonnull optargs) in > + let nns = List.mapi (fun i b -> (i+1, b)) nns in > + let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in > + let nns : string list = List.map string_of_int nns in > + pr "\n LIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat "," nns) For generated code, it is just as easy to cope with either style (we can strip a layer of () if we want a varargs format). > > let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true) > cbargs = > @@ -349,6 +385,19 @@ let > pr "extern \"C\" {\n"; > pr "#endif\n"; > pr "\n"; > + pr "#if defined(__GNUC__)\n"; > + pr "#define LIBNBD_GCC_VERSION \\\n"; > + pr " (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)\n"; > + pr "#endif\n"; > + pr "\n"; > + pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n"; > + pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /* gcc >= 12.0 */\n"; > + pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__ s))\n"; This definition is what requires us to pass in our own (). That is, our end result is going to be one of: __attribute__((__nonnull__(1) )) __attribute__((__nonnull__(1, 2) )) but the difference is whether we must pass exactly one macro argument, and where that argument must include () even when there is only one parameter to be marked (what you coded): LIBNBD_ATTRIBUTE_NONNULL((1)) LIBNBD_ATTRIBUTE_NONNULL((1, 3)) vs. ease-of-use in supplying the () as part of the macro definition itself by using #define MACRO(...) and __VA_ARGS__: LIBNBD_ATTRIBUTE_NONNULL(1) LIBNBD_ATTRIBUTE_NONNULL(1, 3)