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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
