On Feb 24 2014, at 06:09 , Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> wrote: > Mike, > > I have only looked at toolchain.m4. > > First of all, I like the $C_O_FLAG_DEBUG logic. > > Some questions/comments: > * Have you tested the -qnoopt option on xlc?
I copied it from the hotspot makefiles but have not tested it personally. > * This code and comment is not really aligned. Is the comment overly broad, > or is it a mistake in C_O_FLAG_HIGHEST? > # Disable optimization > C_O_FLAG_HIGHEST="$C_O_FLAG_NORM" > C_O_FLAG_HI="$C_O_FLAG_DEBUG" > * Maybe it's worth considering to extract the duplication of > "-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-all --param > ssp-buffer-size=1" into a separate variable, something like > CFLAGS_EXTRA_CHECKS or whatever. The same argument is also valid for the > fastdebug extra switches, I think, even the amount of code duplication is not > as high. Hm. Actually. In fact, from what I can tell this change will add > these extra flags on all compilers, even though they are GCC only (?). So > breaking out these flags into a variable, that can be set to empty if not > using gcc, is probably not just a matter of style but a matter of correctness. I sort of followed this advice, see what you think of how it is now. > * I'd also have a preference for moving the "compiler supports -Og" check > outside the actual flag setting. Maybe you can do the checking first and set > a variable indicating the availablility of this flag, e.g. > GCC_SUPPORTS_OG_FLAG, and then just check on that. The point here is that > it's hard as it is to see the pattern between different optimization levels > and compilers, but the more the code resembles a simple structured assignment > matrix, the easier it is to see it. With checking code like this in the > midst, it's easier to get lost. Done. Mike
