On Tue, 16 Jan 2024 12:52:13 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:

>> make/autoconf/flags-cflags.m4 line 124:
>> 
>>> 122:     fi
>>> 123: 
>>> 124:     if test "x$DEBUG_PREFIX_CFLAGS" != x; then
>> 
>> Should this test not be inside the "if $ALLOW_ABSOLUTE_PATHS_IN_OUTPUT == 
>> false" block?
>> 
>> edit: Ah, I see, this is a consequence of the variables set in that if block.
>
> The logic of this is a bit strange. I think what we really want to express is:
> 
> if ! ALLOW_ABSOLUTE_PATHS_IN_OUTPUT and -fdebug-prefix-map is a valid option 
> for gcc then
>   set -fdebug-prefix-map for both workspace_root and gcc system libraries
> endif
> 
> Now this connection is kind of lost. Maybe the easiest way to do this is just 
> to do the FLAGS_COMPILER_CHECK_ARGUMENTS separately, and reading the result 
> of that check as a boolean variable in a separate if statement, since nesting 
> much code inside a `IF_TRUE:` block can be hard to read. Or move the setting 
> of the workspace_root as well to DEBUG_PREFIX_MAP_GCC_INCLUDE_PATHS, perhaps 
> renaming it in the process, and calling it from `IF_TRUE:`.

@magicus  Yes, good point the condition is sort of hidden as it is.
I've moved the DEBUG_PREFIX_MAP_GCC_INCLUDE_PATHS setting prior to the compiler 
option check, so the whole constructed DEBUG_PREFIX_CFLAGS is verified, which 
is probably how it should be.

Updated... What do you think?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17399#discussion_r1453460028

Reply via email to