aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by dak@gnu.org)

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by dak@gnu.org)

jonas.hahnfeld
Generally looks fine to me, you only might want to revisit indention:
aclocal.m4 uses 4 spaces instead of tabs.

https://codereview.appspot.com/575860044/

Reply | Threaded
Open this post in threaded view
|

Re: aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by dak@gnu.org)

David Kastrup
On 2020/03/20 14:54:36, hahnjo wrote:
> Generally looks fine to me, you only might want to revisit indention:
aclocal.m4
> uses 4 spaces instead of tabs.

Uh, right.  Done.

https://codereview.appspot.com/575860044/

Reply | Threaded
Open this post in threaded view
|

Re: aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by dak@gnu.org)

jonas.hahnfeld
In reply to this post by jonas.hahnfeld
Reply | Threaded
Open this post in threaded view
|

Re: aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by dak@gnu.org)

Dev mailing list
In reply to this post by jonas.hahnfeld
LGTM, and some minor nits.


https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode625
aclocal.m4:625: AC_ARG_VAR(GUILE_FLAVOR, AS_HELP_STRING([],
What about breaking the line here to get shorter lines?

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode630
aclocal.m4:630: AC_ARG_VAR(GUILE_CONFIG, [guile-config executable,
obsoleted by pkgconfig/GUILE_FLAVOR])dnl
Ditto.

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode640
aclocal.m4:640: AC_MSG_ERROR([\$GUILE_CONFIG info guileversion failed:
$tmp_GUILE_FLAVOR])
For consistency I suggest to add `;;` here, too.

https://codereview.appspot.com/575860044/

Reply | Threaded
Open this post in threaded view
|

Re: aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by dak@gnu.org)

David Kastrup
In reply to this post by jonas.hahnfeld

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode625
aclocal.m4:625: AC_ARG_VAR(GUILE_FLAVOR, AS_HELP_STRING([],
On 2020/03/21 05:41:05, lemzwerg wrote:
> What about breaking the line here to get shorter lines?

Acknowledged.

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode640
aclocal.m4:640: AC_MSG_ERROR([\$GUILE_CONFIG info guileversion failed:
$tmp_GUILE_FLAVOR])
On 2020/03/21 05:41:05, lemzwerg wrote:
> For consistency I suggest to add `;;` here, too.

Ugh.  Seems like a case where programming style has converged to using
;; as phrase terminator rather than as phrase separator in violation of
the Bourne shell being in the Algol family (and not requiring ";" before
")" either, for example).  Since it specifies fallover behavior into the
next phrase (different fallovers would be ";&" and ";;&"), this is
really ugly.

I'll admit that it is also the practice in this file elsewhere with the
exception of "esac esac".

Will do.

https://codereview.appspot.com/575860044/