On Mon, 18 Jun 2012, Iain Buclaw wrote: > [PATCH 1/4]: > The D compiler frontend > - gcc/d
Only selectively reviewed, but here are some comments: > diff -Naur gcc-4.8-20120617/gcc/d/asmstmt.cc gcc-4.8/gcc/d/asmstmt.cc > --- gcc-4.8-20120617/gcc/d/asmstmt.cc 1970-01-01 01:00:00.000000000 +0100 > +++ gcc-4.8/gcc/d/asmstmt.cc 2012-06-05 13:42:09.044876794 +0100 > @@ -0,0 +1,2731 @@ > +// asmstmt.cc -- D frontend for GCC. > +// Originally contributed by David Friedman > +// Maintained by Iain Buclaw > + > +// GCC is free software; you can redistribute it and/or modify it under Every file more than ten lines long needs a copyright notice as well as the license notice. See <http://www.gnu.org/prep/maintain/html_node/Copyright-Notices.html> for instructions, including the case of multiple copyright holders - though if there are any significant (more than fifteen lines of copyrightable text or so) contributors not assigning copyright to the FSF then special approval from the FSF will be needed to include the front end. I would say that the files in dfrontend/ need copyright and license notices as well, though not necessarily in exactly GNU form. Thus, you will need to get Digital Mars to approve appropriate notices for those files (aav.c is the first I see that's lacking such a notice but is long enough to need one; likewise async.c, gnuc.c, speller.c; rmem.c just says "All Rights Reserved" and needs a proper license notice like other files; likewise rmem.h). > +#ifdef TARGET_80387 > +#include "d-asm-i386.h" > +#else > +#define D_NO_INLINE_ASM_AT_ALL > +#endif Ugh. We want to move away from target macros, and this isn't even a proper target macro. It would be better to define target hooks for the D inline asm support - possibly with a D-specific hook structure, like the C hooks structure. (Even if you avoid needing copyright assignments for the front end itself, such hook implementations will probably need to be assigned.) > +/* Apple GCC extends ASM_EXPR to five operands; cannot use build4. */ I don't see why that should be in the least relevant to a contribution to FSF GCC. If you can do things in a more natural way in FSF GCC, then do so. Each function in the GCC-specific parts of the code should have a comment on it, explaining the semantics of the function, its operands and its return value if any. For new code in GCC, it's better to use snprintf than sprintf. > +extern void decode_options (struct gcc_options *, struct gcc_options *, Please use appropriate headers rather than local declarations of GCC functions. > +// d-bi-attr.h -- D frontend for GCC. This file looks like it's largely copied from elsewhere in GCC. In such a case, please work out a better way to refactor the code so that it can be shared rather than duplicated. (Again, such common code will no doubt need full copyright assignments.) I don't know whether your assignment "Assigns Past and Future Changes to the GNU D Compiler (GDC)" covers changes elsewhere in GCC. But I expect a general assignment for GCC to be needed for any refactoring involved in adapting common code for use in D. (And such refactoring would be a new contribution so there shouldn't be any issues with unknown previous contributors without assignments - those would only arise if significant amounts of previously written D front-end code are being moved into common code.) > +#if D_VA_LIST_TYPE_VOIDPTR Please avoid #if conditionals on anything that could be a target property. It's generally better to use "if" conditionals instead of #if, so that all cases are checked for syntax in all compiles. I see #if conditions on defines such as "V2" and "V1" as well. Unless something is an *existing* target macro or configure macro in GCC, use "if" conditions and ensure that the macro is defined to true or false values (rather than defined or not defined). But if a macro is always defined, or never defined, then just avoiding the conditionals may be better. The gcc/d/dfrontend/readme.txt says: > +These sources are free, they are redistributable and modifiable > +under the terms of the GNU General Public License (attached as gpl.txt), > +or the Artistic License (attached as artistic.txt). But that license is GPLv2. We need an explicit notice (approved by the copyright holder) saying that *any later version* may be used. If Digital Mars wishes to license the separately maintained dfrontend/ code under GPLv2+ rather than GPLv3+, that's fine, just like the gofrontend/ code is under a permissive license - but it needs to be explicit that any later version may be used. I haven't studied the details of the dfrontend/ code. But if you are to follow the Go model - separately maintained code for the front end proper that may be used verbatim in multiple compilers, with the code outside dfrontend/ doing everything related to interfacing with GCC, and only what's related to interfacing with GCC - then the > +/* NOTE: This file has been patched from the original DMD distribution to > + work with the GDC compiler. > + > + Modified by David Friedman, December 2006 > +*/ comments suggest something problematic - for there to be a genuine separation you should have a single externally maintained distribution that is not inherently GCC-specific. I don't know what the nature of the changes for GCC is - but they should just be changes in areas such as portability or modularity, not involving direct use of GCC interfaces from the front end (for example). I don't know how, if at all, the front end proper handles internationalization of diagnostics - but you should avoid po/exgettext accidentally extracting them for translation in the "gcc" textdomain unless that's what we determine we want. > +// d-gcc-includes.h -- D frontend for GCC. > +// GMP is C++-aware, so we cannot included it in an extern "C" block. > +#include "gmp.h" > + > +// Conflicting definitions between stdio.h and libiberty.h over the throw() > +#define HAVE_DECL_ASPRINTF 1 > + > +extern "C" { > +#include "config.h" > +#include "system.h" An unconditional extern "C" wrapper is definitely wrong here. See the logic used for Go to decide whether GCC interfaces are C or C++ - and include config.h outside the wrapper in any case. config.h must *always* be included before any system headers (such as gmp.h) as it may define feature test macros. Defining HAVE_DECL_ASPRINTF is also wrong - it's a property of the system; if you have libiberty issues, you need to fix them in libiberty. > +#include "debug.h" Why? Please include a comment explaining why this include is needed in a front end. > +#include "function.h" Likewise. > +#include "rtl.h" Does this actually work? If so, how have you worked around the poisoning if IN_GCC_FRONTEND is defined? Front ends should never need to include middle-end headers such as rtl.h. > +#include "except.h" Likewise. > +#include "expr.h" Likewise. Generally, please check that each header is really needed, and unless it's widely included in front ends add a comment explaining why you need it from a front end. > +#undef RET What defines this? > +// Apple makes 'optimize' a macro Comments about Apple are irrelevant here. (But, "optimize" is a macro as a standard matter in GCC. Just avoid using that identifier in problematic ways in your front end, instead of using #undef.) > +// d-incpath.cc -- D frontend for GCC. > +// Originally contributed by heromyth who based off 'incpath.c'. As usual, avoid copying code, instead refactoring as needed. > diff -Naur gcc-4.8-20120617/gcc/d/dmd-script gcc-4.8/gcc/d/dmd-script > +# the Free Software Foundation; either version 2 of the License, or GPLv3+ would be consistent with other files.... > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA Please use URLs not out-of-date postal addresses. > diff -Naur gcc-4.8-20120617/gcc/d/dmd-script.1 gcc-4.8/gcc/d/dmd-script.1 The primary documentation of any GNU program should be Texinfo, not a man page. (You may generate a man page using contrib/texi2pod.pl.) > diff -Naur gcc-4.8-20120617/gcc/d/d-objfile.cc gcc-4.8/gcc/d/d-objfile.cc > +#include <math.h> Why is this needed? > +#include <limits.h> Already included by system.h. > diff -Naur gcc-4.8-20120617/gcc/d/d-spec.c gcc-4.8/gcc/d/d-spec.c This file looks like yet another duplicate with edits of various existing files. I made various postings in September 2010 in the "Objective C/C++ Compiler Drivers" thread that discussed how driver code might be refactored. Note that VEC can now be used in the driver, which may simplify some of the things I discussed. This refactoring isn't needed in the same way as for the other cases of copied and modified code, given how many duplicate drivers we already have, but it's still a good idea. > diff -Naur gcc-4.8-20120617/gcc/d/gdc.1 gcc-4.8/gcc/d/gdc.1 Same comments as for the other manpage. > diff -Naur gcc-4.8-20120617/gcc/d/gdc_alloca.h gcc-4.8/gcc/d/gdc_alloca.h alloca is already handled through libiberty.h, you shouldn't need anything special in your front end. > diff -Naur gcc-4.8-20120617/gcc/d/GDC.html gcc-4.8/gcc/d/GDC.html We don't generally have random HTML pages in the GCC source tree. > diff -Naur gcc-4.8-20120617/gcc/d/INSTALL gcc-4.8/gcc/d/INSTALL Moving to keep in GCC means you should no longer need any special installation instructions - or any support code for GCC versions before current trunk. > diff -Naur gcc-4.8-20120617/gcc/d/INSTALL.html gcc-4.8/gcc/d/INSTALL.html See above. > diff -Naur gcc-4.8-20120617/gcc/d/Make-lang.in gcc-4.8/gcc/d/Make-lang.in > +D_LANGUAGE_VERSION=2 If you want to configure versions they should be configured via -std= or similar options when the compiler is run, rather than determined at compile time. > +D_EXTRA_DEFINES += -D_GNU_SOURCE=1 config.h should already have this from AC_USE_SYSTEM_EXTENSIONS. > +ifeq ($(target_os), mingw32) > +D_EXTRA_DEFINES += -D__USE_MINGW_ANSI_STDIO > +endif I don't see anything testing this define, so it appears to be for the system libc - in which case it would relate to the *host* and *build* systems (whichever the particular compilation is for), not the *target*. > +# This should be configured > +ifeq ($(host), $(target)) You certainly need better explanation of why existing logic to determine directories doesn't work, and what you are trying to achieve. > + d/dmd/aav.h d/dmd/aggregate.h d/dmd/aliasthis.h d/dmd/arraytypes.h \ Where does the dmd/ directory come from? > +ifeq ($(if $(findstring solaris,$(build)),T,$(if $(findstring > solaris,$(host)),T)),T) > +D_CC_FLAGS += -Wcast-align Certainly needs more explanation of what's specific to Solaris. > +# Create the compiler driver for D. > +$(D_DRIVER_NAME)$(exeext): $(D_GCC_OBJS) $(D_DRIVER_OBJS) $(EXTRA_GCC_OBJS) > $(D_EXTRA_SPEC_LIBS) $(LIBDEPS) > + $(CC) $(ALL_CFLAGS) $(LDFLAGS) -o $@ \ I think you mean +$(LINKER) not $(CC). The '+' is I think related to LTO, the use of $(LINKER) is needed to link appropriately with host libstdc++ in the presence of various options for building as C++. I don't see d.pdf or d.html targets in your Make-lang.in - make sure you're up to date with the full current set of hooks front ends should define. > diff -Naur gcc-4.8-20120617/gcc/d/rdmd.1 gcc-4.8/gcc/d/rdmd.1 Same comments as above. -- Joseph S. Myers jos...@codesourcery.com
