On Fri, 12 Sep 2025 14:51:07 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> make/autoconf/flags-other.m4 line 120: >> >>> 118: DESC: [Use SVE when compiling libsleef], >>> 119: AVAILABLE: false, >>> 120: CHECK_AVAILABLE: [ >> >> You are not supposed to both provide AVAILABLE and CHECK_AVAILABLE. Also, if >> you do setup CHECK_AVAILABLE you should always provide a value to the >> AVAILABLE variable; now you don't have an else clause for the cpu and >> toolchain tests. Also, CHECK_AVAILABLE was designed for small and simple >> tests; this is so long it becames hard to read. >> >> Instead I suggest the following: >> Start by setting AARCH64_SVE_AVAILABLE to false. Then do the code that is >> currently in the CHECK_AVAILABLE block, but replace the assignment to >> AVAILABLE with an assignment to AARCH64_SVE_AVAILABLE. (Where it is true, >> since you defaulted it to false you can skip that line). >> >> Then, finally, you can do like this: >> >> UTIL_ARG_ENABLE(NAME: aarch64-sve, DEFAULT: auto, >> RESULT: AARCH64_SVE_ENABLED, >> DESC: [Use SVE when compiling libsleef], >> AVAILABLE: $AARCH64_SVE_AVAILABLE) > > Oh, and also, you can extract a common: > > AC_MSG_RESULT([$AARCH64_SVE_AVAILABLE]) > > after the compile test. This lets you get rid of the else block completely, > and it makes it clear that the AC_MSG_CHECKING is always terminated. (Apart > from being shorter) > You are not supposed to both provide AVAILABLE and CHECK_AVAILABLE. Also, if > you do setup CHECK_AVAILABLE you should always provide a value to the > AVAILABLE variable; now you don't have an else clause for the cpu and > toolchain tests. Also, CHECK_AVAILABLE was designed for small and simple > tests; this is so long it becames hard to read. > > Instead I suggest the following: Start by setting AARCH64_SVE_AVAILABLE to > false. Then do the code that is currently in the CHECK_AVAILABLE block, but > replace the assignment to AVAILABLE with an assignment to > AARCH64_SVE_AVAILABLE. (Where it is true, since you defaulted it to false you > can skip that line). > > Then, finally, you can do like this: > > ``` > UTIL_ARG_ENABLE(NAME: aarch64-sve, DEFAULT: auto, > RESULT: AARCH64_SVE_ENABLED, > DESC: [Use SVE when compiling libsleef], > AVAILABLE: $AARCH64_SVE_AVAILABLE) > ``` Here is the code evaluating AVAILABLE: # Check if the option is available AVAILABLE=ARG_AVAILABLE # Run the available check block (if any), which can overwrite AVAILABLE. ARG_CHECK_AVAILABLE Which is why I interpreted it as ok to supply both AVAILABLE as a default and AVAILABLE_CHECK. Also I don't agree that this is too big to be inline. I think it's perfectly readable. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27073#discussion_r2344815625
