Re: Go front end review: gofrontend/

[Ian Lance Taylor <iant at google dot com>]

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

> This message covers files inside gofrontend/.

Thanks for the detailed review.


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

All fixed.


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

Fixed.


> 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 %<\\%>");

Fixed, although I didn't attempt to make Expression::report_error and
friends handle markup directly.


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

Done.


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

In expressions.cc it is ADJUST_FIELD_ALIGN.  I removed the #include of
tm_p.h from gogo-tree.cc, but I note in passing that it #include's tm.h
because of OBJECT_FORMAT_ELF.


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

I changed atoi to strtol and added overflow checks.


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

Changed to use %m.


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

O_BINARY added.


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

I implemented the same algorithm as append_file_to_dir.  I don't know
whether it matters but it can't hurt.


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

I have punted on this for now as I'm unsure about the current search
algorithm.  In practice most code uses a .a file or a .gox file, not a
.o file.


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

Those lists come from Unicode 5.2.0.

I don't think Go follows the exact recommendations of that TR.  The Go
spec (http://golang.org/doc/go_spec.html) says

    Identifiers name program entities such as variables and types. An
    identifier is a sequence of one or more letters and digits.  The
    first character in an identifier must be a letter.

unicode_char   = /* an arbitrary Unicode code point */ .
unicode_letter = /* a Unicode code point classified as "Letter" */ .
unicode_digit  = /* a Unicode code point classified as "Digit" */ .

letter        = unicode_letter | "_" .

identifier = letter { letter | unicode_digit } .

The TR seems to recommend accepting some combining characters, but since
they are not classed as "Letter" Go does not currently accept them in
identifiers.


Thanks again for the review.  All changes committed to the  gccgo
branch.

Ian