On Mon, 15 Oct 2012, Gary Funck wrote: > Some UPC-specific configuration options are added to > the top-level configure.ac and gcc/configure.ac scripts.
Any patch that includes new configure options should include the documentation for those options in install.texi. Also please include ChangeLog entries with each patch submission, and a more detailed description of the changes involved and the rationale for them and implementation choices made. Also please see the review comments I sent previously in <http://gcc.gnu.org/ml/gcc-patches/2010-07/msg02136.html> (at least, this patch has a license notice in obsolete form, referring to GPLv2 and giving an FSF postal address instead of a URL). The changes to darwin.c, darwin.h, i386.c, rs6000.c should definitely be given their own rationale and called to the attention of relevant target maintainers, possibly through extracting them into separate patches in the series with meaningful subject lines mentioning the relevant targets, not mixed into what's ostensibly a build-system patch. The configure options need justification for their existence and (given that they exist) for working using #if, given that (as I said before, in the above referenced 2010 message, in <http://gcc.gnu.org/ml/gcc-patches/2011-07/msg00083.html> and <http://gcc.gnu.org/ml/gcc-patches/2011-07/msg00135.html>), "if" conditionals are preferred to #if and it's better for users to be able to choose things when they run the compiler rather than only having it determined by the person building the compiler (so configure options determining defaults are better than configure options that force the compiler to behave in a particular way - even if the option affects runtime library builds, it's still better to have a command-line option and the configure option just affect the default, since then at least people can see how code generation changes with the option and potentially build multilibs with both variants). In general, please make sure that you take account of previous feedback on previous GUPC patch submissions, and explain in your submissions how you have adjusted things for this feedback, or, if you are not making a previously requested change, why you consider the approach taken in your patch to be optimal. The change to ACX_BUGURL in configure.ac is simply wrong. Adding $(C_STUB_OBJS) uses to random language makefiles is very suspicious. stub-objc.o isn't needed by non-C-family front ends. Nor should stub-upc.o be - although I think the whole approach of using stub functions like that is wrong, as discussed. The language-independent compiler should be using language-independent interfaces to process language-independent internal representations; if there are additions to the language-independent IR to handle UPC, then the relevant code to handle them should be built in unconditionally. Front ends should not have their own Makefile.in if at all avoidable, only Make-lang.in - why do you have gcc/upc/Makefile.in? Note that it says "Derived from objc/Makefile.in", which was removed in r37088 (2000-10-27). See what I said in my 2010 review about "careful review for whether it ... takes account of cleanups done in the past decade or so" - you really do need to read through the entirety of the code to be submitted, line by line, and check how it stands up to the conventions followed elsewhere in the compiler and to issues that get raised in reviews of other patches nowadays. (But with rearrangement to be an option to the C front end, I expect you shouldn't need upc/Make-lang.in either.) -- Joseph S. Myers jos...@codesourcery.com
