Go front end review: gofrontend/

[Joseph S. Myers]

This message covers files inside gofrontend/.

My previous comments about extern "C" use around includes, and about when 
system headers are included, apply generally here.  As a more specific 
comment, lex.cc includes <stdint.h>; you can't rely on all hosts having 
this header, but if they do then system.h will already have included it.  
The extern "C" around the include of filenames.h in import.cc is 
suspicious in its own way; libiberty headers such as that should contain 
their own extern "C" declaration, as indeed that one does (and in any case 
system.h already includes filenames.h so you don't need to include it 
again).

expressions.cc in various places passes a diagnostic argument of error_at 
to _().  error_at translates its gmsgid operand and it should not be 
passed to _() a second time.

In most places expressions.cc uses _() on the operand of 
this->report_error.  This is correct for the present definition of 
Expression::report_error which passes the string to a "%s" format.  But 
one "floating point constant truncated to integer" message does not use 
_().  And another message "left operand of %<<-%> must be channel" is 
actually a format string, which isn't going to work correctly with the 
present code.  I think Expression::report_error should pass the received 
string directly as a format string to error_at, so allowing %< and %> to 
work, and should be set up so that _() markup is not needed for exgettext 
to extract its argument for translation.  A similar format string issue 
appears in lex.cc,

          this->error("invalid character after %<\\%>");

You've mentioned that Go allows UTF-8 identifiers.  For printing these in 
diagnostics you need to pass them through identifier_to_locale so that 
they are printed in the user's locale character set, which may not be 
UTF-8.  (When the %E format is used, the core code handles this 
automatically.)

What target macros are the tm_p includes in expressions.cc and 
gogo-tree.cc needed for?  Can the uses of those target macros be 
abstracted from the front-end code?

The front end adds three new uses of atoi, which has undefined behavior if 
the input number is out of range.  Though the use of this function is a 
pre-existing condition (bug 44574) so we can't yet poison it in system.h, 
adding more uses seems bad and they should be easy to avoid.  On a similar 
note, strtol is used in lex.cc with no check for overflow (including a 
value outside the range of linenum_type = unsigned int, the type passed to 
linemap_add).

It is generally preferred to use %m in diagnostics instead of directly 
using strerror; if strerror is needed for some reason (e.g. passing to a 
printf function rather than to the GCC pretty-printing machinery; the 
latter has %m, the former may not), use the xstrerror wrapper instead.  
See my patch <http://gcc.gnu.org/ml/gcc-patches/2010-05/msg02226.html>.

In various places you open what I think may be binary files with O_RDONLY 
- don't you need to use O_BINARY as well (defining it to 0 if not already 
defined) for portability to Windows hosts?

In import.cc you form filenames in directories using a hardcoded '/'.  I 
think using this instead of DIR_SEPARATOR may be safe given that cpplib 
(append_file_to_dir) does it - but do you need the append_file_to_dir 
checks relating to "//"?

Does the ".o" handling in import.cc need to use TARGET_OBJECT_SUFFIX (and 
so a target macro dependency unless you abstract it out or make it a 
hook)?  Do the VMS people have any comments on this?

Where did the lists of Unicode letters and digits in lex.cc come from?  
Some particular Unicode version?  A generator program?  Does Go follow the 
recommendations of ISO/IEC TR 10176:2003 regarding what characters to 
allow in identifiers?  (That TR is freely available from 
<http://standards.iso.org/ittf/PubliclyAvailableStandards/index.html>.)

-- 
Joseph S. Myers
joseph@codesourcery.com