On Wed, 10 Apr 2024 22:14:33 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> That build failure in shared code does not happen with Xcode clang, gcc, or >> Visual Studio, even though none of them appear to have a relevant define or >> include. So the clang variant being used for AIX is different from the Xcode >> clang variant (and maybe others) in its treatment of alloca. Weird! >> >> I can also live with either the macro or the includes where needed. I >> dislike >> conditionally adding the include in globalDefinitions_gcc.hpp. > > Should also remove the `#pragma alloca` in os_aix.cpp. 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1565646578
