Patchwork Support official CLooG.org versions.

login
register
mail settings
Submitter Tobias Grosser
Date Nov. 19, 2010, 7:30 p.m.
Message ID <4CE6D048.2040208@fim.uni-passau.de>
Download mbox | patch
Permalink /patch/72301/
State New
Headers show

Comments

Tobias Grosser - Nov. 19, 2010, 7:30 p.m.
On 11/15/2010 10:54 AM, Andreas Schwab wrote:
> Tobias Grosser<grosser@fim.uni-passau.de>  writes:
>
>> diff --git a/config/cloog.m4 b/config/cloog.m4
>> index 6ed0f1b..4936e8a 100644
>> --- a/config/cloog.m4
>> +++ b/config/cloog.m4
>> @@ -36,7 +36,19 @@ AC_DEFUN([CLOOG_INIT_FLAGS],
>>       [AS_HELP_STRING(
>>         [--with-cloog-lib=PATH],
>>         [Specify the directory for the installed CLooG library])])
>> -
>> +
>> +  AC_ARG_ENABLE(cloog-backend,
>> +    [  --enable-cloog-backend[=backend]
>> +				isl: The cloog.org isl backend
>> +				ppl-legacy: The legacy ppl backend - default
>> +				ppl: The cloog.org ppl backend],
>
> The help string should explain what the option does, referring to the
> option argument (which should be written in upper case).

Andreas,

I propose the attached patch to address your comments.

* config/cloog.m4: Use AS_HELP_STRING and fix description.

OK to commit?

Tobi
Ralf Wildenhues - Nov. 19, 2010, 8:47 p.m.
* Tobias Grosser wrote on Fri, Nov 19, 2010 at 08:30:16PM CET:
> On 11/15/2010 10:54 AM, Andreas Schwab wrote:
> >Tobias Grosser writes:
> >>--- a/config/cloog.m4
> >>+++ b/config/cloog.m4
> >>@@ -36,7 +36,19 @@ AC_DEFUN([CLOOG_INIT_FLAGS],
> >>      [AS_HELP_STRING(
> >>        [--with-cloog-lib=PATH],
> >>        [Specify the directory for the installed CLooG library])])
> >>-
> >>+
> >>+  AC_ARG_ENABLE(cloog-backend,
> >>+    [  --enable-cloog-backend[=backend]
> >>+				isl: The cloog.org isl backend
> >>+				ppl-legacy: The legacy ppl backend - default
> >>+				ppl: The cloog.org ppl backend],
> >
> >The help string should explain what the option does, referring to the
> >option argument (which should be written in upper case).

> OK to commit?

OK with nits addressed.

Thanks,
Ralf

> Subject: [PATCH] config/cloog.m4: Use AS_HELP_STRING and fix description

> --- a/config/cloog.m4
> +++ b/config/cloog.m4
> @@ -38,10 +38,10 @@ AC_DEFUN([CLOOG_INIT_FLAGS],
>        [Specify the directory for the installed CLooG library])])
>  
>    AC_ARG_ENABLE(cloog-backend,
> -    [  --enable-cloog-backend[=backend]
> -				isl: The cloog.org isl backend
> -				ppl-legacy: The legacy ppl backend - default
> -				ppl: The cloog.org ppl backend],
> +    [AS_HELP_STRING(
> +      [--enable-cloog-backend[=backend]],

Please use either
         [--enable-cloog-backend[[=BACKEND]]],

or
         [--enable-cloog-backend@<:@=BACKEND@:>@],

so the brackets are rendered in the output.  Please upper-case only the
metasyntactic variables, not the actual values that are to be used:

> +      [Set the CLooG backend used to either ISL, PPL or PPL-LEGACY.
> +      If not specified PPL-LEGACY is selected.])],

Also, I think messages should not be capitalized nor end with period;
and, since they get refilled anyway, you could shorten, e.g.:

         [set the CLooG BACKEND used to either isl, ppl, or ppl-legacy
          (default)])],

Please run autoconf and ./configure --help to check for sane output
before committing.  Thanks.

>      [ if   test "x${enableval}" = "xisl"; then
>  	cloog_backend=isl
>        elif test "x${enableval}" = "xppl"; then
Andreas Schwab - Nov. 22, 2010, 9:29 a.m.
Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

>> --- a/config/cloog.m4
>> +++ b/config/cloog.m4
>> @@ -38,10 +38,10 @@ AC_DEFUN([CLOOG_INIT_FLAGS],
>>        [Specify the directory for the installed CLooG library])])
>>  
>>    AC_ARG_ENABLE(cloog-backend,
>> -    [  --enable-cloog-backend[=backend]
>> -				isl: The cloog.org isl backend
>> -				ppl-legacy: The legacy ppl backend - default
>> -				ppl: The cloog.org ppl backend],
>> +    [AS_HELP_STRING(
>> +      [--enable-cloog-backend[=backend]],
>
> Please use either
>          [--enable-cloog-backend[[=BACKEND]]],

IMHO this is better (the whole argument is a fixed string):

           [[--enable-cloog-backend[=BACKEND]]],

Andreas.
Tobias Grosser - Nov. 24, 2010, 2:24 p.m.
On 11/22/2010 04:29 AM, Andreas Schwab wrote:
> Ralf Wildenhues<Ralf.Wildenhues@gmx.de>  writes:
>
>>> --- a/config/cloog.m4
>>> +++ b/config/cloog.m4
>>> @@ -38,10 +38,10 @@ AC_DEFUN([CLOOG_INIT_FLAGS],
>>>         [Specify the directory for the installed CLooG library])])
>>>
>>>     AC_ARG_ENABLE(cloog-backend,
>>> -    [  --enable-cloog-backend[=backend]
>>> -				isl: The cloog.org isl backend
>>> -				ppl-legacy: The legacy ppl backend - default
>>> -				ppl: The cloog.org ppl backend],
>>> +    [AS_HELP_STRING(
>>> +      [--enable-cloog-backend[=backend]],
>>
>> Please use either
>>           [--enable-cloog-backend[[=BACKEND]]],
>
> IMHO this is better (the whole argument is a fixed string):
>
>             [[--enable-cloog-backend[=BACKEND]]],

I just tried this and autoconf is generating the exact same configure 
file for this. Do you want me to change it anyways?

Cheers
Tobi
Paolo Bonzini - Nov. 24, 2010, 3:12 p.m.
On Wed, Nov 24, 2010 at 15:24, Tobias Grosser <grosser@fim.uni-passau.de> wrote:
> On 11/22/2010 04:29 AM, Andreas Schwab wrote:
>>
>> Ralf Wildenhues<Ralf.Wildenhues@gmx.de>  writes:
>>
>>>> --- a/config/cloog.m4
>>>> +++ b/config/cloog.m4
>>>> @@ -38,10 +38,10 @@ AC_DEFUN([CLOOG_INIT_FLAGS],
>>>>        [Specify the directory for the installed CLooG library])])
>>>>
>>>>    AC_ARG_ENABLE(cloog-backend,
>>>> -    [  --enable-cloog-backend[=backend]
>>>> -                               isl: The cloog.org isl backend
>>>> -                               ppl-legacy: The legacy ppl backend -
>>>> default
>>>> -                               ppl: The cloog.org ppl backend],
>>>> +    [AS_HELP_STRING(
>>>> +      [--enable-cloog-backend[=backend]],
>>>
>>> Please use either
>>>          [--enable-cloog-backend[[=BACKEND]]],
>>
>> IMHO this is better (the whole argument is a fixed string):
>>
>>            [[--enable-cloog-backend[=BACKEND]]],
>
> I just tried this and autoconf is generating the exact same configure file
> for this. Do you want me to change it anyways?

I have no preference, but I'll let Ralf and/or Andreas decide.

Paolo
Ralf Wildenhues - Nov. 25, 2010, 6:37 a.m.
* Paolo Bonzini wrote on Wed, Nov 24, 2010 at 04:12:32PM CET:
> On Wed, Nov 24, 2010 at 15:24, Tobias Grosser wrote:
> > On 11/22/2010 04:29 AM, Andreas Schwab wrote:
> >> Ralf Wildenhues writes:
> >>
> >>>> --- a/config/cloog.m4
> >>>> +++ b/config/cloog.m4
> >>>> @@ -38,10 +38,10 @@ AC_DEFUN([CLOOG_INIT_FLAGS],
> >>>>        [Specify the directory for the installed CLooG library])])
> >>>>
> >>>>    AC_ARG_ENABLE(cloog-backend,
> >>>> -    [  --enable-cloog-backend[=backend]
> >>>> -                               isl: The cloog.org isl backend
> >>>> -                               ppl-legacy: The legacy ppl backend -
> >>>> default
> >>>> -                               ppl: The cloog.org ppl backend],
> >>>> +    [AS_HELP_STRING(
> >>>> +      [--enable-cloog-backend[=backend]],
> >>>
> >>> Please use either
> >>>          [--enable-cloog-backend[[=BACKEND]]],
> >>
> >> IMHO this is better (the whole argument is a fixed string):
> >>
> >>            [[--enable-cloog-backend[=BACKEND]]],
> >
> > I just tried this and autoconf is generating the exact same configure file
> > for this. Do you want me to change it anyways?
> 
> I have no preference, but I'll let Ralf and/or Andreas decide.

Oh, I don't have a strong preference either way either.  I guess maximal
quoting is probably a bit more consistent, but I'm not sure I would
invest a lot of time into making the GCC code base very consistent here.
If somebody else wants to do that (in stage 1) however ...

So yes, I'm fine with making the change, if you also sync to src.

Thanks,
Ralf

Patch

From be959d16334626e12238e6e3119635b48cc6b087 Mon Sep 17 00:00:00 2001
From: Tobias Grosser <grosser@fim.uni-passau.de>
Date: Fri, 19 Nov 2010 14:27:31 -0500
Subject: [PATCH] config/cloog.m4: Use AS_HELP_STRING and fix description

---
 config/cloog.m4 |    8 ++++----
 configure       |    5 ++---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/config/cloog.m4 b/config/cloog.m4
index 96ebd4d..2a846cf 100644
--- a/config/cloog.m4
+++ b/config/cloog.m4
@@ -38,10 +38,10 @@  AC_DEFUN([CLOOG_INIT_FLAGS],
       [Specify the directory for the installed CLooG library])])
 
   AC_ARG_ENABLE(cloog-backend,
-    [  --enable-cloog-backend[=backend]
-				isl: The cloog.org isl backend
-				ppl-legacy: The legacy ppl backend - default
-				ppl: The cloog.org ppl backend],
+    [AS_HELP_STRING(
+      [--enable-cloog-backend[=backend]],
+      [Set the CLooG backend used to either ISL, PPL or PPL-LEGACY.
+      If not specified PPL-LEGACY is selected.])],
     [ if   test "x${enableval}" = "xisl"; then
 	cloog_backend=isl
       elif test "x${enableval}" = "xppl"; then
diff --git a/configure b/configure
index b1235c2..1d1993d 100755
--- a/configure
+++ b/configure
@@ -1453,9 +1453,8 @@  Optional Features:
   --enable-build-with-cxx build with C++ compiler instead of C compiler
   --disable-ppl-version-check    disable check for PPL version
   --enable-cloog-backend=backend
-				isl: The cloog.org isl backend
-				ppl-legacy: The legacy ppl backend - default
-				ppl: The cloog.org ppl backend
+                          Set the CLooG backend used to either ISL, PPL or
+                          PPL-LEGACY. If not specified PPL-LEGACY is selected.
   --disable-cloog-version-check
                           disable check for CLooG version
   --enable-lto            enable link time optimization support
-- 
1.7.1