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.

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

[Mike Frysinger]

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


signature.asc
Description: Digital signature

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: 
 # @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()

[Ulrich Mueller]

> 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


pgpANfbJC5E4o.pgp
Description: PGP signature

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

[rindeal]

> On Sun, 15 May 2016 21:35:41 +0200
> rindeal  wrote:
>
>> > Dnia 15 maja 2016 15:31:29 CEST, Jan Chren  
>> > 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()

[Michał Górny]

On Sun, 15 May 2016 21:35:41 +0200
rindeal  wrote:

> > Dnia 15 maja 2016 15:31:29 CEST, Jan Chren  
> > 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



pgplBHfgT7mV0.pgp
Description: OpenPGP digital signature

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

[rindeal]

> Dnia 15 maja 2016 15:31:29 CEST, Jan Chren  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]

Dnia 15 maja 2016 15:31:29 CEST, Jan Chren  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)

[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