Go front end review: files outside gofrontend/

[Joseph S. Myers]

I'll try to review the Go front-end files.  This message reviews those 
outside the gofrontend/ subdirectory, from 
svn://gcc.gnu.org/svn/gcc/branches/gccgo/gcc/go r166404:

Review of Make-lang.in:

> # C++ support.  This should really be in the top level Makefile.
> 
> ALL_CXXFLAGS = $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wc++-compat, $(ALL_COMPILERFLAGS)) $(CXXFLAGS)

So put it there?  Isn't this relevant to general C++ building support?

> # Build Go export info generator.
> INTERNAL_CFLAGS += -DGO_DEBUGGING_INFO

What code is conditioned on this define?

> GCCGO_OBJS = $(GCC_OBJS) gospec.o intl.o prefix.o version.o
> gccgo$(exeext): $(GCCGO_OBJS) $(EXTRA_GCC_OBJS) $(LIBDEPS)
> 	$(CC) $(ALL_CFLAGS) $(LDFLAGS) -o $@ \
> 	  $(GCCGO_OBJS) $(EXTRA_GCC_OBJS) $(LIBS)

Shouldn't this use $(LINKER) and $(ALL_LINKERFLAGS)?

> go1$(exeext): $(GO_OBJS) attribs.o $(BACKEND) $(LIBDEPS)
> 	$(CXX) $(STATIC_LIBSTDCXX) $(ALL_CXXFLAGS) $(LDFLAGS) -o $@ \
> 	      $(GO_OBJS) attribs.o $(BACKEND) $(LIBS) $(BACKENDLIBS)

If someone uses the various configure options such as 
--with-host-libstdcxx to control linking with libstdc++ (and to link with 
a static libstdc++ with a compiler predating -static-libstdc++, say), how 
will that interact with this link command?  (The expectation with 
--with-host-libstdcxx is that linking ends up using the C compiler to 
avoid the driver doing the linking implicitly adding its own notion of the 
C++ standard library.)

> # Create a version of the g++ driver which calls the cross-compiler.

Not g++....

> gccgo-cross$(exeext): gccgo$(exeext)
> 	-rm -f gccgo-cross$(exeext)
> 	cp gccgo$(exeext) gccgo-cross$(exeext)

I don't actually see anything using this binary.

The install rules below:

> go.install-common: installdirs
> 	-rm -f $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)
> 	-$(INSTALL_PROGRAM) gccgo$(exeext) $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)
> 	-chmod a+x $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)

are generally more sensible than the cargo-culted rules in other 
directories that involve checking for a -cross binary and possibly 
installing a non-functioning copy of the driver in $(gcc_tooldir)/bin/.  
But if you have these rules you shouldn't still have the -cross binary.  
And I think the *correct* rules everywhere ought to involve installing the 
$(target_noncanonical)-{gcc,g++,gccgo,etc.} driver everywhere (though most 
languages install one for both native and cross, this patch would not 
install one for gccgo for native) and not installing any copies in 
$(gcc_tooldir)/bin/ - furthermore, at least the $(INSTALL_PROGRAM) 
commands should not ignore errors.

> go.install-pdf:
> go.html:

The install-html hook appears to be missing.

> GO_SYSTEM_H = go/go-system.h $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) \
> 	$(DIAGNOSTIC_CORE_H) $(INPUT_H)

Limiting front-end dependence on target macros and tm.h is generally a 
good idea; putting tm.h into a header included everywhere is a less good 
idea.  What are the actual target macro dependencies in the Go front end?  
Can you contain them within files outside the gofrontend/ directory?

Review of go-c.h:

> extern int go_enable_dump(const char*);

Existing GCC and GNU style has the space before the open parenthesis.  I 
think this should apply at least to all the C/C++ code outside the 
gofrontend/ subdirectory.

Review of go-lang.c:

>   /* The sizetype may be "unsigned long" or "unsigned long long".  */
>   if (TYPE_MODE (long_unsigned_type_node) == ptr_mode)
>     size_type_node = long_unsigned_type_node;
>   else if (TYPE_MODE (long_long_unsigned_type_node) == ptr_mode)
>     size_type_node = long_long_unsigned_type_node;
>   else
>     size_type_node = long_unsigned_type_node;

This exists in other front ends so isn't an obstacle to inclusion, but 
it's also ugly.  Hopefully at some point we'll transition macros such as 
SIZE_TYPE to evaluate to enum values rather than strings (and then convert 
them to hooks), so avoiding the need for such hacks for non-C-family front 
ends to work out a suitable sizetype, and also avoiding Fortran needing 
its own type name parsing code (get_typenode_from_name) to implement the C 
bindings.

>   set_sizetype(size_type_node);

Missing space.

> /* Initialize before parsing options.  */
> 
> static void
> go_langhook_init_options (
>     unsigned int argc ATTRIBUTE_UNUSED,
>     struct cl_decoded_option *decoded_options ATTRIBUTE_UNUSED)

As previously noted, use the init_options_struct hook as far as possible.

>   /* We default to always showing a column number.  */
>   flag_show_column = 1;

This is the GCC default now.  The variable (field in global_options) only 
exists for initialization in toplev.c:general_init and for the sake of 
reporting option state; the setting in global_dc is used for everything 
else.

> void
> go_preserve_from_gc(tree t)

Missing space.

Review of go-system.h:

> #include <tr1/unordered_map>

So you're requiring more than just the intersection of C++98 with the C++ 
language supported by some baseline GCC version (the agreed language for 
GCC-in-C++, I think)?  How widespread is <tr1/unordered_map> support in 
C++ implementations?  What was the first GCC version with it?  How hard 
would it be to provide a C++ equivalent of libiberty that provides such 
facilities on systems that don't have them (which seems analogous to how 
we handle missing C library facilities)?

Or do we believe this is not an issue as long as we don't use TR1 
facilities in code required for the C and C++ compilers, because we can 
have stricter requirements for building cross compilers and for native 
compilers a new libstdc++ will be built with the stage 1 C++ compiler?  
(If so, the documentation of prerequisites in install.texi still needs to 
say what GCC version is needed to build a Go cross compiler.)

> extern "C"

If GCC is built as C++, don't the various GCC headers you're including 
inside extern "C" actually describe C++ interfaces rather than C ones?  
How does this work?

> #include "tm.h"

(Remarked upon above.)

Review of gospec.c:

> #ifndef THREAD_LIBRARY
> #define THREAD_LIBRARY "pthread"
> #endif
> #ifndef THREAD_LIBRARY_PROFILE
> #define THREAD_LIBRARY_PROFILE THREAD_LIBRARY
> #endif
> 
> #ifndef LIBGO
> #define LIBGO "go"
> #endif
> #ifndef LIBGOBEGIN
> #define LIBGOBEGIN "gobegin"
> #endif
> #ifndef LIBGO_PROFILE
> #define LIBGO_PROFILE LIBGO
> #endif

By using #ifndef you're implicitly creating new target macros.  But I 
don't see any documentation of these macros in tm.texi.in on the branch.  
Do you really need to allow for these things to be changed by targets?  If 
so, the macros need documenting; if not, don't use #ifndef.

>   /* If nonzero, the user gave us the `-p' or `-pg' flag.  */
>   int saw_profile_flag = 0;

New code should be using bool for booleans, not int (and false/true not 
0/1).

I have no comments on files outside gofrontend/ not mentioned above.

-- 
Joseph S. Myers
joseph@codesourcery.com