This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Go front end review: files outside gofrontend/


"Joseph S. Myers" <joseph@codesourcery.com> writes:

> 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:

Thanks very much for the review.


> 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?

Removed.


>> # Build Go export info generator.
>> INTERNAL_CFLAGS += -DGO_DEBUGGING_INFO
>
> What code is conditioned on this define?

None anymore.  Removed.


>> 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)?

Yes.  Changed.


>> 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.)

I have a pending configure patch which will permit removing
$(STATIC_LIBSTDCXX), and will let this link be entirely controlled by
--with-host-libstdcxx, --with-stage1-ldflags, and friends.

http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01639.html


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

Removed.


>> 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.

Removed.


> 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.

This rule does install gccgo for native, so I'm not sure what you mean
there.  I changed this to not ignore the exit status of the
$(INSTALL_PROGRAM), and I removed the chmod since most installs don't do
it.


>> go.install-pdf:
>> go.html:
>
> The install-html hook appears to be missing.

Added.


>> 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?

I moved the tm.h dependency to the specific files which require it.

> 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.

Fixed.


> 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.

I didn't do anything here.


>>   set_sizetype(size_type_node);
>
> Missing space.

Fixed.

>> /* 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.

Fixed.


>>   /* 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.

Removed setting flag_show_column.


>> void
>> go_preserve_from_gc(tree t)
>
> Missing space.

Fixed.


> 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.)

I changed this to use configure tests for what is available, along the
same lines as gold.


>> 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?

It works now.


> 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.

Fixed.


>>   /* 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).

Fixed.

All these changes are committed to gccgo branch.

Thanks again for the review.

Ian


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]