On Mon, 15 Apr 2024 13:10:22 GMT, Martin Doerr <mdo...@openjdk.org> wrote:
>> It was too bad that I did not see and review this change in the makefiles.
>> :-(
>>
>> While you guys could have gone either way, I strongly dislike the choice to
>> include a redefinition in the makefiles. If this really should be done, we
>> should introduced a new variable to carry such changes, instead of
>> piggybacking it with the OS defines. :-( But, I don't think it should be
>> done at all.
>>
>> There are several reasons why this is a inferior solution:
>>
>> 1) It does not follow prior examples. We have tried hard before not do
>> things like this, but rather pass flags as defines (e.g. `-DREDEFINE_ALLOCA`
>> had been better)
>> 2) It does not scale. If we start in effect allowing code in the command
>> line, there is no clear limit anymore what should be placed in the source
>> code files and what should be placed on the command line.
>> 3) It messes up command lines. Keeping command lines as short as reasonable
>> possible is a goal we try to strive for. In this case, there is also the `'`
>> inside them (which I don't understand why), which is just begging for
>> quoting/escaping problems, making command lines hard to copy/paste, send to
>> different systems (like logging) etc.
>>
>> I'd really like to see a follow-up PR that moves this away from the command
>> line define and into a source code file instead.
>
> Can we unconditionally `#include <alloca.h>` in all files which use `alloca`?
> Or does that disturb any platform?
I don't think it does. From the documentation I've checked, `#include
<alloca.h>` is valid everywhere. I'm not sure why the fact that it is a
compiler-builtin is relevant here. The man page for alloca on Linux says:
SYNOPSIS
#include <alloca.h>
void *alloca(size_t size);
which I take it as this is the formally correct way to use alloca. If it
happens to work without the include file, that's just coincidence, and not a
sign that we should remove `#include <alloca.h>` everywhere. @kimbarrett What
do you say?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1566964500
