On 19 June 2012 17:20, Joseph S. Myers <jos...@codesourcery.com> wrote:
> 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).
>
I have raised this with Walter, and the licensing has been fixed in
all frontend code.
>> +#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.)
>
This code has been removed entirely.
>> +/* 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.
>
Now use build5, similar to other frontends.
> 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.
>
Am working on this.
> For new code in GCC, it's better to use snprintf than sprintf.
>
Have fixed this. Thanks.
>> +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.)
>
It's copied as including c-common.c / .h causes problems with a fair
number of references pulled in that need to be stubbed out - also,
some GCC function attributes that we use do not make any sense to have
in D code (eg: gnu_inline, artificial, cleanup). It could certainly
be possible though ... Will need to review this in more detail.
>> +#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.
>
Have remove this from the gdc glue.
> 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.
>
Again, have raised with Walter, and have fixed the wording of the
license 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.
>
The D frontend is completely independent of the GCC backend, and any
alterations are purely for portability (eg, the use of real_t rather
than long double for the representation of floats). There is no
internationalisation of diagnostics in the D frontend so are not
subject to any conflicts with po/exgettext.
>> +// 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.
>
Have re-arranged the order of included headers to fix this.
>> +#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?
>
Have removed all of the above.
>> +// 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.)
>
Likewise, have removed it as is in fact no longer required. The
"optimize" #undef remains for the time being as it conflicts with the
name of a member in the D frontend sources. If the D frontend
followed the C++ Coding Conventions as outlined in
gcc.gnu.org/wiki/CppConventions then this wouldn't be an issue.
Though I don't think it has an obligation to being essentially
disconnected from calling any GCC code.
>> +// 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.
>
Will certainly look into this, but as I recall, the code in
d-incpath.cc differs from the source it was based off in a few ways,
eg: searches for DPATH rather than CPATH, etc. If this can be changed
in incpath.c to be more transitive to other frontends using it, then
great! I'll work something out for that.
>> 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.
>
Have sorted this out.
>> 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.)
>
OK - I will try to find time to move the documentation across.
>> 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.
>
The reason escapes me, but they aren't required at all anymore, so
they have been removed entirely in all GDC sources that had them.
>> 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.
>
Have removed all alloca handling from GDC and replaced with simply
including libiberty.h.
>> 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.
>
Have removed these files.
>> 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.
>
At one point, you had the ability to build GDC using either D version
1 or D version 2 - this is what the V1 and V2 macros were used for.
As D1 will reach end of maintenance at the end of the year, I have
removed this, and all such V1 code from GDC.
>> +D_EXTRA_DEFINES += -D_GNU_SOURCE=1
>
> config.h should already have this from AC_USE_SYSTEM_EXTENSIONS.
>
Removed all extra defines such as these.
>> +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?
>
As above with the explanation of the D1 / D2 builds. The 'dmd' part of
that was substr'd out with the correct path to the D frontend sources.
Also as above, D1 has been removed so corrected this with using
'dfrontend' instead.
>> +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.
>
I think that knowledge has been lost, but there most certainly has
been improvements to cross-platform compatibility from both the D
frontend and GCC since that was put in, so have removed this from the
makefile.
>> +# 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.
>
OK, thanks for spotting this. Have changed it to be LINKER, and added
the missing hooks.
I will try to sort out things I haven't addressed yet in the coming
week. Any changes / commits will be done in our git repository.
https://github.com/D-Programming-GDC/GDC/commits/master
I do have a question though, what is available for the transition of
development from git to svn? Other than a lot of ready and getting
used to the various switches and commands on my part.
--
Iain Buclaw
*(p < e ? p++ : p) = (c & 0x0f) + '0';
