Re: [gentoo-dev] Please review fortran-2.eclass next round

[Mike Frysinger]

> # @ECLASS: fortran-2.eclass

i dont see fortran.eclass.  what's with the "-2" ?

> @BLURB: Packages, which need a fortran compiler should inherit this eclass.

@BLURB: simplify fortran compiler management

>@DESCRIPTION:
> If you need a fortran compiler, inherit this eclass.

If you need a fortran compiler, then you should be inheriting this eclass.

> Optional, it checks for openmp capability of the current fortran compiler
> through FORTRAN_NEED_OPENMP=1.

Optionally, it checks for extended capabilities based on the variable options 
selected in the ebuild.

> Only phase function exported is pkg_pretend and pkg_setup.

The only phase functions exported are pkg_pretend and pkg_setup.

> Need help? Ask the sci team.

seems redundant what with the @MAINTAINER field ...

> # @ECLASS-VARIABLE: FORTRAN_NEED_OPENMP
> # @DESCRIPTION:
> # Set FORTRAN_NEED_OPENMP=1 in order to test FC for openmp capabilities

Set to "1" in order to automatically have the eclass abort if the fortran 
compiler lacks openmp support.

> #
> # Default is 0

have the code list the default and then you don't need this.  simply use:
: ${FORTRAN_NEED_OPENMP:=0}

> # @ECLASS-VARIABLE: FORTRAN_STANDARD
> # @DESCRIPTION:
> # Set this, if a special dialect needs to be support. Generally not needed.

drop the comma and add "ed" to support

> # Valid settings are any combination of
> #
> # FORTRAN_STANDARD="77 90 95 2003"

Valid settings are any combination of: 77 90 95 2003

> # Defaults to FORTRAN_STANDARD="77" which is sufficient for most cases.

same as the openmp code; list the default after the comment:
: ${FORTRAN_STANDARD:=77}

> DEPEND="virtual/fortran"
> RDEPEND="${DEPEND}"

i'm not sure that RDEPEND is correct.  do all fortran compilers additionally 
require the fortran compiler to be available at runtime ?

> # internal function

use the @INTERNAL tag and proper eclass documentation

> local filebase=${T}/test-fortran

there is $(emktemp) ... but this is probably fine ...

> 	[[ -z ${fcomp} ]] && die "_compile_test() needs at least one argument"

probably want: [[ $# -eq 0 ]]

>	[[ -f "${filebase}.f${fdia}" ]] || _write_testsuite

no need to quote with [[...]]

>	[[ -f "${filebase}.f${fdia}" ]] || _write_testsuite
>
>	${fcomp} "${filebase}.f${fdia}" -o "${filebase}-f${fdia}" >&/dev/null
>	local ret=$?
>
>	rm -f "${filebase}-f${fdia}"

seems like you want a local var that is like:
	${filebase}.f${fdia}
and then you write the output to:
	${filebase}.f${fdia}.x
and then you don't need to repeat this logic multiple times ...

>		$(tc-getFC "$@") ${flag} "${filebase}.f" -o "${filebase}" >&/dev/null

tc-getFC could expand into a bit of logic ... probably better to cache the 
result in a local var

>		local ret=$?

local is a command, and is a bit odd to see inside of a for loop.  please 
hoist it up to the top with the "local flag" decl.

>		(( ${ret} )) || break

a little odd syntax, but i guess it works ...

>	eerror "fortran dialects are support."

supported

>	die "Currently no working fortran compiler is available"

"is available" -> "could be located"

>	[[ -n ${F77} ]] || F77=$(tc-getFC)

: ${F77:=$(tc-getFC)}

although i wonder why you're not using tc-getF77 ...

>		ewarn "The support for EAPI=${EAPI} by the fortran-2.eclass"

why ?  it's causing you no real trouble to do so

>	[[ -n ${F77} ]] || export F77=$(tc-getFC)

same comment as before ...

>	[[ -n ${FC} ]] || export FC=$(tc-getFC)

tc-export FC

> case "${EAPI:-0}" in

no need to quote
-mike