On 7/27/23 19:27, Eric Blake wrote:
> On Thu, Jul 27, 2023 at 01:54:25PM +0200, Laszlo Ersek wrote:
>> On 7/26/23 19:50, Eric Blake wrote:
>>> Use Go-preferred whitespace: TAB for indent, and no space before
>>> opening '(', better use of blank lines. These changes reduce the size
>>> of a diff produced by gofmt, although there are still other places
>>> where we are not idiomatic in what we generate (mainly in lining up
>>> columns of = in const() blocks).
>>>
>>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>>> ---
>>> generator/GoLang.ml | 244 ++++++++++++++++++++++----------------------
>>> 1 file changed, 123 insertions(+), 121 deletions(-)
>>
>> Unfortunately:
>>
>> - This must have been an incredible amount of work for you.
>
> Some automation involved (emacs macros are nice), but yeah, it's
> grunt-work.
>
>>
>> - The many \t escape sequences make the generator source code harder to
>> read.
>
> Agreed. Maybe a better approach would be figuring out an optional
> argument to pr to insist on TAB instead of space indent? Or, as
> mentioned elsewhere, not bothering with TAB at all in OCaml but
> instead figuring out how to run gofmt itself as part of the generation
> pipeline.
>
>>
>> - This patch depends on patch#4 (minimally), so if you modify that one
>> like I've asked, it will create a conflict for this patch. :(
>
> Yep, I've already encountered a number of conflicts as I shuffle
> around patches, trying to pick out which changes are more than just
> whitespace (such as the RBool change). Not terribly difficult, but
> back in the grunt-work category.
>
>>
>> Acked-by: Laszlo Ersek <ler...@redhat.com>
>>
>> Thanks
>> Laszlo
>>
>>>
>>> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
>>> index 77dacadb..a38aa19f 100644
>>> --- a/generator/GoLang.ml
>>> +++ b/generator/GoLang.ml
>>> @@ -175,6 +175,7 @@ let
>>> let print_binding (name, { args; optargs; ret; shortdesc }) =
>>> let cname = camel_case name in
>>>
>>> + pr "\n";
>>> (* Tedious method of passing optional arguments in golang. *)
>>> if optargs <> [] then (
>>> pr "/* Struct carrying optional arguments for %s. */\n" cname;
>>> @@ -182,10 +183,10 @@ let
>>> List.iter (
>>> fun optarg ->
>>> let fname = go_name_of_optarg optarg in
>>> - pr " /* %s field is ignored unless %sSet == true. */\n"
>>> + pr "\t/* %s field is ignored unless %sSet == true. */\n"
>>> fname fname;
>
> I tried:
>
> + pr "\t" ^ "/* %s field is ignored unless %sSet == true. */\n"
> fname fname;
>
> File "GoLang.ml", line 186, characters 8-15:
> 186 | pr "\t" ^ "/* %s field is ignored unless %sSet == true. */\n"
> ^^^^^^^
> Error: This expression has type unit but an expression was expected of type
> string
>
> and:
>
> File "GoLang.ml", line 186, characters 11-71:
> 186 | pr ("\t" ^ "/* %s field is ignored unless %sSet == true. */\n")
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Error: This expression has type string but an expression was expected of type
> ('weak2 -> 'weak3 -> 'weak4, unit, string, unit) format4 =
> ('weak2 -> 'weak3 -> 'weak4, unit, string, string, string, unit)
> CamlinternalFormatBasics.format6
Sigh. :/
"pr" is declared like this [generator/utils.mli]:
val pr : ('a, unit, string, unit) format4 -> 'a
The format string would normally be converted to a "format4" type, which
is a polymorphic type, taking one type variable ('a).
It seems that this automatic conversion/construction, from string to
format4, is performed by a mechanism that is similar to
"Stdlib.format_of_string":
https://v2.ocaml.org/api/Stdlib.html#1_Operationsonformatstrings
There are two catches however:
- Stdlib.format_of_string only works with string *literals*, according
to the documentation,
- Stdlib.format_of_string produces a format6, not a format4.
The documentation recommends "Scanf.format_from_string":
https://v2.ocaml.org/api/Scanf.html#VALformat_from_string
so you could *theoretically* do something like
printf (Scanf.format_from_string ("%d " ^ "%d\n")) 1 2
it still doesn't work though, because Scanf.format_from_string also
produces a format6 (like Stdlib.format_of_string does), but printf
expects a plain "format" (similarly to how "pr" expects "format4", i.e.,
*not* format6).
So this appears unsolvable. "Scanf.format_from_string" produces a
format6, which takes six type variables, but "pr" expects "format4",
which is much less generic:
type ('a, 'b, 'c, 'd) format4 = ('a, 'b, 'c, 'c, 'c, 'd) format6
We'd have to find a way to "narrow" the format6 produced by
"Scanf.format_from_string" into a format4, and I don't think that's
possible (the parameters are not values but types). And I couldn't find
a Printf module function that took a format6.
Ultimately open-coding the \t escape sequences seems the least intrusive...
>
> so those are non-starters. We can get more verbose with:
>
> + pr "\t";
> + pr "/* %s field is ignored unless %sSet == true. */\n"
> fname fname;
>
> but that may not scale as nicely. Go's use of TAB does not specify
> what tabstop it prefers;
well that I find unfathomable; how can any coding style mandate TABs for
indentation if it doesn't explain how wide a TAB should be? For example,
that makes "maximum line width" mostly impossible to define.
> our .editorconfig suggests using a tabstop of
> 4 (instead of the usual 8) when reading a .go file for less visual
> distraction, and which matches with GoLang.ml currently using 4-space
> indentation.
>
> Rich, do you have any ideas on any better approach to take here?
>
>>> - pr " %sSet bool\n" fname;
>>> - pr " %s " fname;
>>> + pr "\t%sSet bool\n" fname;
>>> + pr "\t%s " fname;
>
> I also debated about splitting this patch into two (yes, more
> gruntwork): one to address space before '(', and the other to address
> leading indentation. Lines like this would be touched by both
> patches if I split.
>
I wouldn't insist on such a split. :)
Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
