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

["Michał Górny" <mgorny@gentoo.org>]

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)