* [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() @ 2016-05-15 13:31 Jan Chren 2016-05-15 15:59 ` Michał Górny 0 siblings, 1 reply; 10+ messages in thread From: Jan Chren @ 2016-05-15 13:31 UTC (permalink / raw To: gentoo-dev; +Cc: 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-15 13:31 [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() Jan Chren @ 2016-05-15 15:59 ` Michał Górny 2016-05-15 19:35 ` rindeal 0 siblings, 1 reply; 10+ messages in thread From: Michał Górny @ 2016-05-15 15:59 UTC (permalink / raw To: gentoo-dev, Jan Chren; +Cc: Jan Chren 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) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-15 15:59 ` Michał Górny @ 2016-05-15 19:35 ` rindeal 2016-05-15 19:41 ` Michał Górny ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: rindeal @ 2016-05-15 19:35 UTC (permalink / raw To: Michał Górny; +Cc: gentoo-dev, Jan Chren > 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-15 19:35 ` rindeal @ 2016-05-15 19:41 ` Michał Górny 2016-05-15 20:19 ` rindeal 2016-05-15 23:23 ` Ulrich Mueller 2016-05-16 6:23 ` [gentoo-dev] " Ryan Hill 2 siblings, 1 reply; 10+ messages in thread From: Michał Górny @ 2016-05-15 19:41 UTC (permalink / raw To: rindeal; +Cc: gentoo-dev [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-15 19:41 ` Michał Górny @ 2016-05-15 20:19 ` rindeal 0 siblings, 0 replies; 10+ messages in thread From: rindeal @ 2016-05-15 20:19 UTC (permalink / raw To: Michał Górny; +Cc: rindeal, gentoo-dev > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-15 19:35 ` rindeal 2016-05-15 19:41 ` Michał Górny @ 2016-05-15 23:23 ` Ulrich Mueller 2016-05-16 12:17 ` rindeal 2016-05-16 6:23 ` [gentoo-dev] " Ryan Hill 2 siblings, 1 reply; 10+ messages in thread From: Ulrich Mueller @ 2016-05-15 23:23 UTC (permalink / raw To: gentoo-dev; +Cc: Michał Górny, Jan Chren [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-15 23:23 ` Ulrich Mueller @ 2016-05-16 12:17 ` rindeal 2016-05-20 3:15 ` Mike Frysinger 2016-06-05 12:13 ` rindeal 0 siblings, 2 replies; 10+ messages in thread From: rindeal @ 2016-05-16 12:17 UTC (permalink / raw To: Ulrich Mueller; +Cc: gentoo-dev, Michał Górny, Jan Chren > [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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-16 12:17 ` rindeal @ 2016-05-20 3:15 ` Mike Frysinger 2016-06-05 12:13 ` rindeal 1 sibling, 0 replies; 10+ messages in thread From: Mike Frysinger @ 2016-05-20 3:15 UTC (permalink / raw To: gentoo-dev [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-16 12:17 ` rindeal 2016-05-20 3:15 ` Mike Frysinger @ 2016-06-05 12:13 ` rindeal 1 sibling, 0 replies; 10+ messages in thread From: rindeal @ 2016-06-05 12:13 UTC (permalink / raw To: gentoo-dev; +Cc: Ulrich Mueller, Michał Górny New version in available at https://github.com/gentoo/gentoo/pull/1425. Check it out, it's cool. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [gentoo-dev] Re: [PATCH] flag-o-matic.eclass: bugfix for get-flag() 2016-05-15 19:35 ` rindeal 2016-05-15 19:41 ` Michał Górny 2016-05-15 23:23 ` Ulrich Mueller @ 2016-05-16 6:23 ` Ryan Hill 2 siblings, 0 replies; 10+ messages in thread From: Ryan Hill @ 2016-05-16 6:23 UTC (permalink / raw To: gentoo-dev [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-05 12:14 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-15 13:31 [gentoo-dev] [PATCH] flag-o-matic.eclass: bugfix for get-flag() Jan Chren 2016-05-15 15:59 ` Michał Górny 2016-05-15 19:35 ` rindeal 2016-05-15 19:41 ` Michał Górny 2016-05-15 20:19 ` rindeal 2016-05-15 23:23 ` Ulrich Mueller 2016-05-16 12:17 ` rindeal 2016-05-20 3:15 ` Mike Frysinger 2016-06-05 12:13 ` rindeal 2016-05-16 6:23 ` [gentoo-dev] " Ryan Hill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox