Re: [Minios-devel] [UNIKRAFT PATCHv4 07/43] build: Add a makefile function to check and set flags for valid gcc

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 06.07.2018 11:03, Wei Chen wrote:
> Some times, we will add special flags to GCC to do optimization. For
> instance, we can use this function to check valid GCC and set flags
> to do processors optimization. In order to avoid any definition
> clashes, we define this function in support/build/Makefile.rules.
>
> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> ---
>   support/build/Makefile.rules | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/support/build/Makefile.rules b/support/build/Makefile.rules
> index f71fd6e..261b8f5 100644
> --- a/support/build/Makefile.rules
> +++ b/support/build/Makefile.rules
> @@ -42,6 +42,14 @@ $(if $(call test_gcc_version,$(1),$(2)),,\
>        $(error Require GCC version >= $(1).$(2) found 
> $(CC_VER_MAJOR).$(CC_VER_MINOR)))
>   endef
>   
> +# set_flags_if_gcc_version_ge $gcc_major,$gcc_minor,$flags
> +define set_flags_if_gcc_version_ge =
> +$(call error_if_gcc_version_lt,$(1),$(2))
Oh, I would not expect from the set_flags_if_gcc_version_ge function 
name that it is going to let Make stop execution and returning an error. 
I would expect that it sets only the flags if the condition is met and 
ignore it otherwise. Hum...
> +ASFLAGS-$(1)  += $(3)
> +CFLAGS-$(1)   += $(3)
> +CXXFLAGS-$(1) += $(3)
I had a chat with Sharan and we actually prefer forgetting about a 
function that sets flags to all these three variables. We get the 
feeling that the function is too powerful with the cost not to 
understand anymore what it does.
Instead of doing this:

$(call set_flags_if_gcc_version_ge,4,5,-my_flag)

...we think about a usage flow like this (as equal expression):

$(call error_if_gcc_version_lt,4,5)

ASFLAGS-$(call gcc_version_ge,4,5)  += -my_flag
CFLAGS-$(call gcc_version_ge,4,5)   += -my_flag
CXXFLAGS-$(call gcc_version_ge,4,5) += -my_flag

I know you have to write more for this but we think this is easier to 
get without needing to lookup what the Make functions actually do. 
gcc_version_ge is the function you have called test_gcc_version in your 
previous patch and which result in a `y` if the GCC version is greater 
or equal to the parameters.
I am sorry to suggest you again another change of this. I know that we 
agreed on the system you implemented right now. What do you think?

> +endef
> +
>   
> ################################################################################
>   #
>   # Paths and Filenames
Thanks,

Simon

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel