[gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[Jan Chren]

- fix case:
  - `CFLAGS='-O1 -O2'`
  - `get-flag '-O*'`
  - before `-O1`
  - now `-O2`
- fix case:
  - `CFLAGS='-W1,-O1'`
  - `get-flag '-O*'`
  - before `-W1,O1`
  - now return 1

`get-flag march` == "i686" syntax still works.
---
 eclass/flag-o-matic.eclass | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/eclass/flag-o-matic.eclass b/eclass/flag-o-matic.eclass
index e0b19e9..f670320 100644
--- a/eclass/flag-o-matic.eclass
+++ b/eclass/flag-o-matic.eclass
@@ -535,7 +535,7 @@ strip-unsupported-flags() {
 # @DESCRIPTION:
 # Find and echo the value for a particular flag.  Accepts shell globs.
 get-flag() {
-	local f var findflag="$1"
+	local var findflag="${1}"
 
 	# this code looks a little flaky but seems to work for
 	# everything we want ...
@@ -543,11 +543,16 @@ get-flag() {
 	# `get-flag -march` == "-march=i686"
 	# `get-flag march` == "i686"
 	for var in $(all-flag-vars) ; do
-		for f in ${!var} ; do
-			if [ "${f/${findflag}}" != "${f}" ] ; then
-				printf "%s\n" "${f/-${findflag}=}"
+		# reverse loop
+		set -- ${!var}
+		local i=$#
+		while [ $i -gt 0 ] ; do
+			local f="${!i}"
+			if [ "${f#-${findflag#-}}" != "${f}" ] ; then
+				printf "%s\n" "${f#-${findflag}=}"
 				return 0
 			fi
+			((i--))
 		done
 	done
 	return 1
-- 
2.7.3

Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[Michał Górny]

Dnia 15 maja 2016 15:31:29 CEST, Jan Chren <dev.rindeal@gmail.com> napisał(a):
>- fix case:
>  - `CFLAGS='-O1 -O2'`
>  - `get-flag '-O*'`
>  - before `-O1`
>  - now `-O2`
>- fix case:
>  - `CFLAGS='-W1,-O1'`
>  - `get-flag '-O*'`
>  - before `-W1,O1`
>  - now return 1
>
>`get-flag march` == "i686" syntax still works.

Could you add appropriate test cases, in the tests subdirectory?

>---
> eclass/flag-o-matic.eclass | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
>diff --git a/eclass/flag-o-matic.eclass b/eclass/flag-o-matic.eclass
>index e0b19e9..f670320 100644
>--- a/eclass/flag-o-matic.eclass
>+++ b/eclass/flag-o-matic.eclass
>@@ -535,7 +535,7 @@ strip-unsupported-flags() {
> # @DESCRIPTION:
> # Find and echo the value for a particular flag.  Accepts shell globs.
> get-flag() {
>-	local f var findflag="$1"
>+	local var findflag="${1}"
> 
> 	# this code looks a little flaky but seems to work for
> 	# everything we want ...
>@@ -543,11 +543,16 @@ get-flag() {
> 	# `get-flag -march` == "-march=i686"
> 	# `get-flag march` == "i686"
> 	for var in $(all-flag-vars) ; do
>-		for f in ${!var} ; do
>-			if [ "${f/${findflag}}" != "${f}" ] ; then
>-				printf "%s\n" "${f/-${findflag}=}"
>+		# reverse loop
>+		set -- ${!var}
>+		local i=$#

You are using $ with and without braces inconsistently. Please stick to one form.

>+		while [ $i -gt 0 ] ; do

Please use [[ ]] for conditionals. It has some nice bash magic that makes them whitespace-safe.

>+			local f="${!i}"
>+			if [ "${f#-${findflag#-}}" != "${f}" ] ; then

I know the original code sucked as well but could you replace this with more readable [[ ${f} == -${findflag#-}* ]] or alike (note: not tested).

>+				printf "%s\n" "${f#-${findflag}=}"

It may be a good idea to add a short explanation why you can't use echo here, as a comment.

> 				return 0
> 			fi
>+			((i--))
> 		done
> 	done
> 	return 1


-- 
Best regards,
Michał Górny (by phone)

Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[rindeal]

> Dnia 15 maja 2016 15:31:29 CEST, Jan Chren <dev.rindeal@gmail.com> napisał(a):
>>- fix case:
>>  - `CFLAGS='-O1 -O2'`
>>  - `get-flag '-O*'`
>>  - before `-O1`
>>  - now `-O2`
>>- fix case:
>>  - `CFLAGS='-W1,-O1'`
>>  - `get-flag '-O*'`
>>  - before `-W1,O1`
>>  - now return 1
>>
>>`get-flag march` == "i686" syntax still works.
>
> Could you add appropriate test cases, in the tests subdirectory?

Done

>
>>---
>> eclass/flag-o-matic.eclass | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>>diff --git a/eclass/flag-o-matic.eclass b/eclass/flag-o-matic.eclass
>>index e0b19e9..f670320 100644
>>--- a/eclass/flag-o-matic.eclass
>>+++ b/eclass/flag-o-matic.eclass
>>@@ -535,7 +535,7 @@ strip-unsupported-flags() {
>> # @DESCRIPTION:
>> # Find and echo the value for a particular flag.  Accepts shell globs.
>> get-flag() {
>>-      local f var findflag="$1"
>>+      local var findflag="${1}"
>>
>>       # this code looks a little flaky but seems to work for
>>       # everything we want ...
>>@@ -543,11 +543,16 @@ get-flag() {
>>       # `get-flag -march` == "-march=i686"
>>       # `get-flag march` == "i686"
>>       for var in $(all-flag-vars) ; do
>>-              for f in ${!var} ; do
>>-                      if [ "${f/${findflag}}" != "${f}" ] ; then
>>-                              printf "%s\n" "${f/-${findflag}=}"
>>+              # reverse loop
>>+              set -- ${!var}
>>+              local i=$#
>
> You are using $ with and without braces inconsistently. Please stick to one form.

Done

>
>>+              while [ $i -gt 0 ] ; do
>
> Please use [[ ]] for conditionals. It has some nice bash magic that makes them whitespace-safe.

Although always a number, but done

>
>>+                      local f="${!i}"
>>+                      if [ "${f#-${findflag#-}}" != "${f}" ] ; then
>
> I know the original code sucked as well but could you replace this with more readable [[ ${f} == -${findflag#-}* ]] or alike (note: not tested).

This is just as buggy as my original implementation, I've reworked it
and thanks to the tests I hope it's now correct.

>
>>+                              printf "%s\n" "${f#-${findflag}=}"
>
> It may be a good idea to add a short explanation why you can't use echo here, as a comment.

I've just copied what was there before, `echo` in bash is notoriously
wild, but with this simple string I guess it's ok, so done.

>
>>                               return 0
>>                       fi
>>+                      ((i--))
>>               done
>>       done
>>       return 1
>
>

apart from the tests, the patch now looks like this:

 eclass/flag-o-matic.eclass | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/eclass/flag-o-matic.eclass b/eclass/flag-o-matic.eclass
index e0b19e9..a8a792e 100644
--- a/eclass/flag-o-matic.eclass
+++ b/eclass/flag-o-matic.eclass
@@ -535,7 +535,7 @@ strip-unsupported-flags() {
 # @DESCRIPTION:
 # Find and echo the value for a particular flag.  Accepts shell globs.
 get-flag() {
-   local f var findflag="$1"
+   local var pattern="${1}"

    # this code looks a little flaky but seems to work for
    # everything we want ...
@@ -543,11 +543,21 @@ get-flag() {
    # `get-flag -march` == "-march=i686"
    # `get-flag march` == "i686"
    for var in $(all-flag-vars) ; do
-       for f in ${!var} ; do
-           if [ "${f/${findflag}}" != "${f}" ] ; then
-               printf "%s\n" "${f/-${findflag}=}"
+       # reverse loop
+       set -- ${!var}
+       local i=${#}
+       while [[ ${i} > 0 ]] ; do
+           local arg="${!i}"
+           local needle="-${pattern#-}" # force dash on
+           local haystack="${arg%%=*}" # we're comparing flags, not values
+           if [[ ${haystack##${needle}} == '' ]] ; then
+               local ret
+               # preserve only value if only flag name was provided
+               ret="${arg#-${pattern}=}"
+               echo "${ret}"
                return 0
            fi
+           ((i--))
        done
    done
    return 1
--
2.7.3

Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On Sun, 15 May 2016 21:35:41 +0200
rindeal <dev.rindeal@gmail.com> wrote:

> > Dnia 15 maja 2016 15:31:29 CEST, Jan Chren <dev.rindeal@gmail.com> napisał(a):  
> >>+                      local f="${!i}"
> >>+                      if [ "${f#-${findflag#-}}" != "${f}" ] ; then  
> >
> > I know the original code sucked as well but could you replace this with more readable [[ ${f} == -${findflag#-}* ]] or alike (note: not tested).  
> 
> This is just as buggy as my original implementation, I've reworked it
> and thanks to the tests I hope it's now correct.

It is still unreadable. The point is, we use bash here, so please use
bash features (i.e. == with wildcards) to do comparison rather than
limited shell-style stripping of variables.

> >>+                              printf "%s\n" "${f#-${findflag}=}"  
> >
> > It may be a good idea to add a short explanation why you can't use echo here, as a comment.  
> 
> I've just copied what was there before, `echo` in bash is notoriously
> wild, but with this simple string I guess it's ok, so done.

I meant you should add a comment that you can't use echo because flags
like '-n' or '-e' would confuse it :-P.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[rindeal]

> On Sun, 15 May 2016 21:35:41 +0200
> rindeal <dev.rindeal@gmail.com> wrote:
>
>> > Dnia 15 maja 2016 15:31:29 CEST, Jan Chren <dev.rindeal@gmail.com> napisał(a):
>> >>+                      local f="${!i}"
>> >>+                      if [ "${f#-${findflag#-}}" != "${f}" ] ; then
>> >
>> > I know the original code sucked as well but could you replace this with more readable [[ ${f} == -${findflag#-}* ]] or alike (note: not tested).
>>
>> This is just as buggy as my original implementation, I've reworked it
>> and thanks to the tests I hope it's now correct.
>
> It is still unreadable. The point is, we use bash here, so please use
> bash features (i.e. == with wildcards) to do comparison rather than
> limited shell-style stripping of variables.

The thing is that "== with wildcards" cannot be used, because a) it's
too greedy and b) the wildcards in ${pattern} won't expand.

>
>> >>+                              printf "%s\n" "${f#-${findflag}=}"
>> >
>> > It may be a good idea to add a short explanation why you can't use echo here, as a comment.
>>
>> I've just copied what was there before, `echo` in bash is notoriously
>> wild, but with this simple string I guess it's ok, so done.
>
> I meant you should add a comment that you can't use echo because flags
> like '-n' or '-e' would confuse it :-P.

Ok, I've fixed it and added tests for such edge cases.

Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

>>>>> On Sun, 15 May 2016, rindeal  wrote:

[Looks like your mailer is broken. All the tabs in your patch have
been mangled and appear as spaces.]

> +       # reverse loop
> +       set -- ${!var}
> +       local i=${#}
> +       while [[ ${i} > 0 ]] ; do
> +           local arg="${!i}"

Using the positional parameters looks needlessly complicated here.
Why not use an array, like this (untested):

	local -a flags=(${!var})
	for (( i=${#flags[@]}-1; i>=0; i-- )); do

Below you can use ${flags[i]} instead of ${arg} then.

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

[gentoo-dev] Re: [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[Ryan Hill]

[-- Attachment #1: Type: text/plain, Size: 172 bytes --]

On Sun, 15 May 2016 21:35:41 +0200
rindeal <dev.rindeal@gmail.com> wrote:

> apart from the tests, the patch now looks like this:

Please posts the tests too.

-- 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 951 bytes --]

Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[rindeal]

> [Looks like your mailer is broken. All the tabs in your patch have
> been mangled and appear as spaces.]
>
>> +       # reverse loop
>> +       set -- ${!var}
>> +       local i=${#}
>> +       while [[ ${i} > 0 ]] ; do
>> +           local arg="${!i}"
>
> Using the positional parameters looks needlessly complicated here.
> Why not use an array, like this (untested):
>
>         local -a flags=(${!var})
>         for (( i=${#flags[@]}-1; i>=0; i-- )); do
>
> Below you can use ${flags[i]} instead of ${arg} then.

Done.

I've also changed comments and added examples section to docs.

So this is what it looks like now:

diff --git a/eclass/flag-o-matic.eclass b/eclass/flag-o-matic.eclass
index e0b19e9..217d33b 100644
--- a/eclass/flag-o-matic.eclass
+++ b/eclass/flag-o-matic.eclass
@@ -534,18 +534,26 @@ strip-unsupported-flags() {
 # @USAGE: <flag>
 # @DESCRIPTION:
 # Find and echo the value for a particular flag.  Accepts shell globs.
+#
+# Example:
+# @CODE
+# CFLAGS="-march=i686 -O1"
+# get-flag -march # outputs "-march=i686"
+# get-flag march  # outputs "i686"
+# get-flag '-O*'  # outputs "-O1"
+# @CODE
 get-flag() {
-   local f var findflag="$1"
-
-   # this code looks a little flaky but seems to work for
-   # everything we want ...
-   # for example, if CFLAGS="-march=i686":
-   # `get-flag -march` == "-march=i686"
-   # `get-flag march` == "i686"
+   local var pattern="${1}"
    for var in $(all-flag-vars) ; do
-       for f in ${!var} ; do
-           if [ "${f/${findflag}}" != "${f}" ] ; then
-               printf "%s\n" "${f/-${findflag}=}"
+       local i flags=( ${!var} )
+       for (( i=${#flags[@]}-1; i>=0; i-- )) ; do
+           local needle="-${pattern#-}" # force dash on
+           local haystack="${flags[i]%%=*}" # we're comparing flags, not values
+           if [[ ${haystack##${needle}} == '' ]] ; then
+               # preserve only value if only flag name was provided
+               local ret="${flags[i]#-${pattern}=}"
+               # ${ret} might contain `-e` or `-n` which confuses echo
+               printf '%s\n' "${ret}"
                return 0
            fi
        done

Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On 16 May 2016 14:17, rindeal wrote:
> So this is what it looks like now:

still missing tests.  see eclass/tests/flag-o-matic.sh.

> -   local f var findflag="$1"
> -
> -   # this code looks a little flaky but seems to work for
> -   # everything we want ...
> -   # for example, if CFLAGS="-march=i686":
> -   # `get-flag -march` == "-march=i686"
> -   # `get-flag march` == "i686"
> +   local var pattern="${1}"

drop the braces for builtin vars

>     for var in $(all-flag-vars) ; do
> -       for f in ${!var} ; do
> -           if [ "${f/${findflag}}" != "${f}" ] ; then
> -               printf "%s\n" "${f/-${findflag}=}"
> +       local i flags=( ${!var} )
> +       for (( i=${#flags[@]}-1; i>=0; i-- )) ; do

omitting spaces doesn't make code faster, it just makes it unreadable.
	for (( i = ${#flags[@]} - 1; i >= 0; --i )) ; do

stick a comment above this for loop explaining why we walk backwards.

> +           local needle="-${pattern#-}" # force dash on

avoid inline comments and make them complete sentences.
	# Make sure we anchor to the leading dash.
	local needle="-${pattern#-}"

> +           local haystack="${flags[i]%%=*}" # we're comparing flags, not values

it's a bit easier to read if you unpack the flag explicitly.
	local flag=${flags[i]}

> +           if [[ ${haystack##${needle}} == '' ]] ; then

use a -z test instead of comparing to ''
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag()

[rindeal]

New version in available at
https://github.com/gentoo/gentoo/pull/1425. Check it out, it's cool.