Re: [Libguestfs] [libnbd PATCH 3/3] generator: indent C argument list 2 spaces relative to function designator
On 4/20/23 16:36, Richard W.M. Jones wrote:
On Thu, Apr 20, 2023 at 04:04:58PM +0200, Laszlo Ersek wrote:
> Change the optional "parens" parameter of "print_arg_list" from
bool to a
> new union type. The new union type has three data constructors, NoParens
> (matching the previous "false" value), ParensSameLine (matching the
> previous "true" value), and a third constructor called
"ParensNewLine",
> which wraps an "int". [ParensNewLine n] stands for the following
> formatting construct (with wrapping):
>
>> ret_type foobar (
>> type1 param1, type2 param2, type3 param3, type4 param4,
>> type5 param5
>> );
>> <---n--->
I would have called it ParensNewLineWithIndent, and I would
have called this:
> +type args_style = NoParens | ParensSameLine | ParensNewLine of int
parens_style or parens_t just to tie it back to the only parameter
where it is used.
I'll adopt these, thanks!
> +let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types
> + ?(parens = ParensSameLine) ?closure_style args optargs =
> + (match parens with
> + | NoParens -> ()
> + | ParensSameLine -> pr "("
> + | ParensNewLine indent ->
> + pr "(\n";
> + for i = 0 to indent + 2 - 1 do
> + pr " "
> + done
libguestfs common/mlstdutils/std_utils.ml has a "spaces" function
which might be worth grabbing for this. The license is compatible
enough, and if not as author I give you permission.
> if wrap then
> pr_wrap ?maxcol ','
> (fun () -> print_arg_list' ?handle ?types ?closure_style args optargs)
> else
> print_arg_list' ?handle ?types ?closure_style args optargs;
> - if parens then pr ")"
> + (match parens with
> + | NoParens -> ()
> + | ParensSameLine -> pr ")"
> + | ParensNewLine indent ->
> + pr "\n";
> + for i = 0 to indent - 1 do
> + pr " "
> + done;
> + pr ")"
> + )
& here.
Well I took the open-coded loop directly from the "pr_wrap"
implementation [generator/utils.ml] :)
...
for i = 0 to wrapping_col-1 do pr " " done;
...
So I guess I should rebase that to the "spaces" function too.
Now, looking up what "spaces" does...
let spaces n = String.make n ' '
OK, that's simple enough. (But, I agree it adds value: "spaces 5" is
much easier to understand than "String.make 5 ' '".)
The rest looks good to me.
Thanks!
Laszlo