12:27 p.m.
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 <eblake(a)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 <lersek(a)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
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; 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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org