Re: [PATCH 02/14] Add D frontend (GDC) implementation.

[Iain Buclaw <ibuclaw at gdcproject dot org>]

On 14 October 2018 at 17:29, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> [Sorry if this turns out to do be a dup]
>
> Iain Buclaw <ibuclaw@gdcproject.org> writes:
>> On 18 September 2018 at 02:33, Iain Buclaw <ibuclaw@gdcproject.org> wrote:
>>> This patch adds the D front-end implementation, the only part of the
>>> compiler that interacts with GCC directly, and being the parts that I
>>> maintain, is something that I can talk about more directly.
>>>
>>> For the actual code generation pass, that converts the front-end AST
>>> to GCC trees, most parts use a separate Visitor interfaces to do a
>>> certain kind of lowering, for instance, types.cc builds *_TYPE trees
>>> from AST Type's.  The Visitor class is part of the DMD front-end, and
>>> is defined in dfrontend/visitor.h.
>>>
>>> There are also a few interfaces which have their headers in the DMD
>>> frontend, but are implemented here because they do something that
>>> requires knowledge of the GCC backend (d-target.cc), does something
>>> that may not be portable, or differ between D compilers
>>> (d-frontend.cc) or are a thin wrapper around something that is managed
>>> by GCC (d-diagnostic.cc).
>>>
>>> Many high level operations result in generation of calls to D runtime
>>> library functions (runtime.def), all with require some kind of runtime
>>> type information (typeinfo.cc).  The compiler also generates functions
>>> for registering/deregistering compiled modules with the D runtime
>>> library (modules.cc).
>>>
>>> As well as the D language having it's own built-in functions
>>> (intrinsics.cc), we also expose GCC builtins to D code via a
>>> `gcc.builtins' module (d-builtins.cc), and give special treatment to a
>>> number of UDAs that could be applied to functions (d-attribs.cc).
>>>
>>>
>>> That is roughly the high level jist of how things are currently organized.
>>>
>>> ftp://ftp.gdcproject.org/patches/v4/02-v4-d-frontend-gdc.patch
>>>
>>
>> https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00587.html
>>
>> During the last round, the last comment given was that the reviewer
>> wasn't going to dive deep into this code, as it's essentially
>> converting between the different representations and is code that I'd
>> be maintaining.
>>
>> As this is code that other gcc maintainers will be potentially looking
>> after as well, I'd like any glaring problems to be dealt with
>> immediately.
>
> I won't claim that I dived deep into the code either, since with a patch
> of this size that would take weeks.  But FWIW I read it through trying
> to scan for anything that stood out.
>
> I think the patch is OK besides the below.  The comments are in patch
> order and so mix very trivial stuff with things that seem more fundamental.
> I think the only real blocker is the point below about translations.
>

Having a quick look at the fix-ups, I would say all look reasonable.

>> +/* Define attributes that are mutually exclusive with one another.  */
>> +static const struct attribute_spec::exclusions attr_noreturn_exclusions[] =
>> +{
>> +  ATTR_EXCL ("noreturn", true, true, true),
>
> Why does noreturn exclude itself?  Probably worth a comment if deliberate.
>

That does seem wrong indeed, so I'll remove it.

>> +static tree
>> +handle_const_attribute (tree *node, tree ARG_UNUSED (name),
>> +                     tree ARG_UNUSED (args), int ARG_UNUSED (flags),
>> +                     bool * ARG_UNUSED (no_add_attrs))
>> +{
>> +  tree type = TREE_TYPE (*node);
>> +
>> +  /* See FIXME comment on noreturn in c_common_attribute_table.  */
>
> Seems strange to be referencing c-family code here, especially since
> there's no corresponding comment for handle_noreturn_attribute and
> also no FIXME comment in the D version of the table.  Think we should
> just drop the comment above.
>
> It's unfortunate that there's so much cut-&-paste with c-attribs.c,
> but there again, the current split between target-independent code
> (attribs.*) and frontend code (c-attribs.*) isn't very clear-cut,
> so I don't think this should hold up acceptance.
>

Attributes handlers present only for built-in functions would have
been initially copied from LTO if memory serves right.

> I'm going to review it as new code though, so:
>

All suggestions seem reasonable to me.

>> +  /* For now, we need to tell the D frontend what platform is being targeted.
>> +     This should be removed once the frontend has been fixed.  */
>> +  if (strcmp (ident, "linux") == 0)
>> +    global.params.isLinux = true;
>> +  else if (strcmp (ident, "OSX") == 0)
>> +    global.params.isOSX = true;
>> +  else if (strcmp (ident, "Windows") == 0)
>> +    global.params.isWindows = true;
>> +  else if (strcmp (ident, "FreeBSD") == 0)
>> +    global.params.isFreeBSD = true;
>> +  else if (strcmp (ident, "OpenBSD") == 0)
>> +    global.params.isOpenBSD = true;
>> +  else if (strcmp (ident, "Solaris") == 0)
>> +    global.params.isSolaris = true;
>> +  else if (strcmp (ident, "X86_64") == 0)
>> +    global.params.is64bit = true;
>
> Probably worth adding a comment to say why only X86_64 causes is64bit
> to be set, and to say that it's OK to silently ignore other strings
> (assuming it is).
>

The reference D compiler (DMD) only supports x86 and x86_64.

These global.params.isXXX parameters are something inherited from the
DMD front-end, and its been a long term goal of mine to weed them all
out so this is no longer necessary.


>> +/* Build nodes that are used by the D front-end.
>> +   These are distinct from C types.  */
>> +
>> +static void
>> +d_build_d_type_nodes (void)
>> +{
>> +  /* Integral types.  */
>> +  byte_type_node = make_signed_type (8);
>> +  ubyte_type_node = make_unsigned_type (8);
>> +
>> +  short_type_node = make_signed_type (16);
>> +  ushort_type_node = make_unsigned_type (16);
>> +
>> +  int_type_node = make_signed_type (32);
>> +  uint_type_node = make_unsigned_type (32);
>> +
>> +  long_type_node = make_signed_type (64);
>> +  ulong_type_node = make_unsigned_type (64);
>
> It's a bit confusing for the D type to be long_type_node but the C/ABI type
> to be long_integer_type_node.  The D type is surely an integer too. :-)
> With this coming among the handling of built-in functions, it initially
> looked related, and I was wondering how it was safe on ILP32 systems
> before realising the difference.
>
> Maybe prefixing them all with "d_" would be too ugly, but it would at
> least be good to clarify the comment to say that these are "distinct
> type nodes" (rather than just distinct definitions, as I'd initially
> assumed) and that they're not used outside the frontend, or by the C
> imports.
>

If prefixing with "d_", perhaps dropping the "_node" would make them
sufficiently not ugly (d_long_type, d_uint_type, d_byte_type).

Will add a comment though regardless, as that's something you can't
have too much of.

>> +/* Return the GCC location for the D frontend location LOC.   */
>> +
>> +location_t
>> +get_linemap (const Loc& loc)
>
> FWIW, I think GCC style is to add a space before "&" rather than
> after it, as for pointers.  But since you use this style consistently
> (and I'm guessing would instinctively use it in later work), it's
> probably better to leave it as-is.  The go interface similarly
> has local conventions regarding things like spaces before "(".
>
>> +tree
>> +d_array_value (tree type, tree len, tree data)
>> +{
>> +  /* TODO: Assert type is a D array.  */
>
> Just curious, what makes asserting difficult enough to be a TODO?
>

Should be nothing, as all D dynamic array types should be marked as
TYPE_DYNAMIC_ARRAY starting from early last year.

Looking at the history, it looks like that comment pre-dates 2009.

I'll test the corrective action and remove the comment.


>> +      /* If we are taking the address of a decl that can never be null,
>> +      then the return result is always true.  */
>> +      if (DECL_P (TREE_OPERAND (expr, 0))
>> +       && (TREE_CODE (TREE_OPERAND (expr, 0)) == PARM_DECL
>> +           || TREE_CODE (TREE_OPERAND (expr, 0)) == LABEL_DECL
>> +           || !DECL_WEAK (TREE_OPERAND (expr, 0))))
>> +     return boolean_true_node;
>
> Does something warn about this?
>

Nothing warns about this.  Can look into adding if this is a concern.


>> +tree
>> +convert (tree type, tree expr)
>> ...
>> +    case COMPLEX_TYPE:
>> +      if (TREE_CODE (etype) == REAL_TYPE && TYPE_IMAGINARY_FLOAT (etype))
>> +     ret = build2 (COMPLEX_EXPR, type,
>> +                   build_zero_cst (TREE_TYPE (type)),
>> +                   convert (TREE_TYPE (type), expr));
>> +      else
>> +     ret = convert_to_complex (type, e);
>> +      goto maybe_fold;
>> ...
>> +    maybe_fold:
>> +      if (!TREE_CONSTANT (ret))
>> +     ret = fold (ret);
>> +      return ret;
>
> etc.  Any reason not to use fold_build* here and in subroutines (for all
> cases, not just COMPLEX_TYPE), rather than fold at the end?  Or is the
> !TREE_CONSTANT deliberately trying to avoid folding constants that
> fold_build* would normally fold?   build* routines set TREE_CONSTANT
> for operations with constant operands as well as constant literals,
> so is the code trying to avoid folding those?
>
> The routines use fold_build* in some places and plain build* with
> a goto in others, but it wasn't obvious why.  Seems like it deserves
> a comment.
>

Most places where fold_build* vs. build* have been a trial and error
process.  I couldn't comment on any individual one.  Though will run
your suggestion through the testsuite, as it is a nice clean-up.

>> +/* Helper routine for all error routines.  Reports a diagnostic specified by
>> +   KIND at the explicit location LOC, where the message FORMAT has not yet
>> +   been translated by the gcc diagnostic routines.  */
>> +
>> +static void ATTRIBUTE_GCC_DIAG(3,0)
>> +d_diagnostic_report_diagnostic (const Loc& loc, int opt, const char *format,
>> +                             va_list ap, diagnostic_t kind, bool verbatim)
>> +{
>> +  va_list argp;
>> +  va_copy (argp, ap);
>> +
>> +  if (loc.filename || !verbatim)
>> +    {
>> +      rich_location rich_loc (line_table, get_linemap (loc));
>> +      diagnostic_info diagnostic;
>> +      char *xformat = expand_format (format);
>> +
>> +      diagnostic_set_info (&diagnostic, xformat, &argp, &rich_loc, kind);
>
> How does this work with translation?  xgettext will only see the original
> format string, not the result of expand_format.  Do you have some scripting
> to do the same format mangling when collecting the translation strings?
> Same concern:
>

These diagnostic routines handle errors coming from the dmd front-end,
which are not translated - all sources are listed under po/EXCLUDES in
another patch.


>> +void ATTRIBUTE_GCC_DIAG(2,0)
>> +verror (const Loc& loc, const char *format, va_list ap,
>> +     const char *p1, const char *p2, const char *)
>> +{
>> +  if (!global.gag || global.params.showGaggedErrors)
>> +    {
>> +      char *xformat;
>> +
>> +      /* Build string and emit.  */
>> +      if (p2 != NULL)
>> +     xformat = xasprintf ("%s %s %s", p1, p2, format);
>> +      else if (p1 != NULL)
>> +     xformat = xasprintf ("%s %s", p1, format);
>> +      else
>> +     xformat = xasprintf ("%s", format);
>
> ...with the concatenation here.
>
> It looks at first glance like the callers to d_diagnostic_report_diagnostic
> should be doing the conversion themselves via _(format), before doing
> any concatenation.  Then d_diagnostic_report_diagnostic can use
> expand_format on the translated string and use
> diagnostic_set_info_translated.  But I'm not an expert on this stuff.
>
> Also, the function should document what p1 and p2 are.  Do they
> need to be translated?
>
> Does xgettext get to see all frontend error messages?  Haven't got
> to the later patches yet.
>
> Please add an overview comment somewhere in the diagnostic routines
> explaining how translation works wrt errors that come from the D
> frontend.
>

OK, will do.

>> +/* Write a little-endian 32-bit VALUE to BUFFER.  */
>> +
>> +void
>> +Port::writelongLE (unsigned value, void *buffer)
>> +{
>> +    unsigned char *p = (unsigned char*) buffer;
>> +
>> +    p[0] = (unsigned) value;
>> +    p[1] = (unsigned) value >> 8;
>> +    p[2] = (unsigned) value >> 16;
>> +    p[3] = (unsigned) value >> 24;
>> +}
>> ...
>> +/* Write a big-endian 32-bit VALUE to BUFFER.  */
>> +
>> +void
>> +Port::writelongBE (unsigned value, void *buffer)
>> +{
>> +    unsigned char *p = (unsigned char*) buffer;
>> +
>> +    p[0] = (unsigned) value >> 24;
>> +    p[1] = (unsigned) value >> 16;
>> +    p[2] = (unsigned) value >> 8;
>> +    p[3] = (unsigned) value;
>> +}
>
> Overindented bodies.  Missing space before "*" in "(unsigned char*)"
> in all these functions.
>
> Obviously this stuff assumes host CHAR_BIT == 8, but let's be realistic :-)
> Is it also used in ways that require the target BITS_PER_UNIT to be 8
> as well?  That could realistically be a different value (and we've had
> ports like that in the past).
>

These read(long|word)(BE|LE) functions should only ever be used when
reading the BOM of a UTF-16 or UTF-32 file.

I've done a grep, and the write(long|word)(BE|LE) are no longer used
by the dmd frontend, so there's little point keeping them around.

If there's any utility in libiberty or another location then I'd be
more than happy to delegate this action to that here.


>> +      else if (empty_modify_p (TREE_TYPE (op0), op1))
>> +     {
>> +       /* Remove any copies of empty aggregates.  Also drop volatile
>> +          loads on the RHS to avoid infinite recursion from
>> +          gimplify_expr trying to load the value.  */
>> +       gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
>> +                      is_gimple_lvalue, fb_lvalue);
>> +       if (TREE_SIDE_EFFECTS (op1))
>> +         {
>> +           if (TREE_THIS_VOLATILE (op1)
>> +               && (REFERENCE_CLASS_P (op1) || DECL_P (op1)))
>> +             op1 = build_fold_addr_expr (op1);
>> +
>> +           gimplify_and_add (op1, pre_p);
>> +         }
>> +       *expr_p = TREE_OPERAND (*expr_p, 0);
>> +       ret = GS_OK;
>> +     }
>
> It seems odd to gimplify the address of a volatile decl.  When would
> that have meaningful side-effects?
>
> What goes wrong if you don't have this block and let the gimplifier
> handle empty modify_exprs normally?  The comments explain clearly what
> the code is trying to do, but it isn't obvious why this needs to be
> treated as a special case.
>

I'll remove it and report back.  I suspect I won't hit any problems
though as "volatile" has been removed from the D language.  There
should be no decls marked as such anymore.

>> +/* Implements the lang_hooks.get_alias_set routine for language D.
>> +   Get the alias set corresponding the type or expression T.
>> +   Return -1 if we don't do anything special.  */
>> +
>> +static alias_set_type
>> +d_get_alias_set (tree t)
>> +{
>> +  /* Permit type-punning when accessing a union, provided the access
>> +     is directly through the union.  */
>> +  for (tree u = t; handled_component_p (u); u = TREE_OPERAND (u, 0))
>> +    {
>> +      if (TREE_CODE (u) == COMPONENT_REF
>> +       && TREE_CODE (TREE_TYPE (TREE_OPERAND (u, 0))) == UNION_TYPE)
>> +     return 0;
>> +    }
>> +
>> +  /* That's all the expressions we handle.  */
>> +  if (!TYPE_P (t))
>> +    return get_alias_set (TREE_TYPE (t));
>> +
>> +  /* For now in D, assume everything aliases everything else,
>> +     until we define some solid rules.  */
>> +  return 0;
>> +}
>
> Any reason for not returning 0 unconditionally?  Won't the queries
> deferred to get_alias_set either return 0 directly or come back here
> and get 0?
>
> Might be worth adding a comment referencing build_vconvert, which stood
> out as a source of what in C would be alias violations.
>

All previous attempts at doing more with this has caused silent
corruption at runtime, the existence of build_vconvert likely doesn't
help either.

Although it isn't in the spec, D should be "strict aliasing".  But
there's a lot of production code that looks like `intvar = *(int
*)&floatvar;` that is currently expected to just work.

By rule of thumb in D, the C behaviour should be followed if it looks
like C code.  The only places where we deviate are made to be a
compiler error.

>> +/* Used to represent a D inline assembly statement, an intermediate
>> +   representation of an ASM_EXPR.  Reserved for future use and
>> +   implementation yet to be defined.  */
>> +DEFTREECODE (IASM_EXPR, "iasm_expr", tcc_statement, 5)
>
> Do you need to reserve it?  The codes are local to the frontend
> and you can add new ones whenever you want, so it would be good
> to leave this out if possible.
>

At this point in time, I never intend to add support for DMD-style
inline asm, so it's better removed.

>> +    /* The LHS expression could be an assignment, to which it's operation gets
>> +       lost during gimplification.  */
>> +    if (TREE_CODE (lhs) == MODIFY_EXPR)
>> +      {
>> +     lexpr = compound_expr (lexpr, lhs);
>> +     lhs = TREE_OPERAND (lhs, 0);
>> +      }
>
> s/it's/its/.  But the code looks a bit odd -- won't you end up with
> double evaluation of the lhs of the MODIFY_EXPR?  Or is the creator of
> the MODIFY_EXPR already guaranteed to have wrapped the lhs in a SAVE_EXPR
> where necessary?  Probably worth a comment if so.
>

It should be guaranteed to be saved if there's a side effect, but that
can be easily checked.

>> +  /* Build a power expression, which raises its left operand to the power of
>> +     its right operand.   */
>> +
>> +  void visit (PowExp *e)
>> +  {
>> ...
>> +    /* If type is int, implicitly convert to double.  This allows backend
>> +       to fold the call into a constant return value.  */
>> +    if (e->type->isintegral ())
>> +      powtype = double_type_node;
>
> Seems dangerous if pow is defined as an integer operation.
> E.g. ((1 << 62) | 1) ** 1 should be (1 << 62) | 1, which isn't
> representable in double.
>

I'll have to check when this could ever be the case and leave an explanation.

The operation: `x ^^ y` in D is rewritten as `std.math.pow(x, y)` if
there's any templates that match argument constraints.  All integral
pow() operations in user code should never reach here, so I suspect
this is something special for CTFE or static var initialisers.


>> +/* Clear the DECL_BUILT_IN_CLASS flag on the function in CALLEXP.  */
>> +
>> +static void
>> +clear_intrinsic_flag (tree callexp)
>> +{
>> +  tree decl = CALL_EXPR_FN (callexp);
>> +
>> +  if (TREE_CODE (decl) == ADDR_EXPR)
>> +    decl = TREE_OPERAND (decl, 0);
>> +
>> +  gcc_assert (TREE_CODE (decl) == FUNCTION_DECL);
>> +
>> +  DECL_INTRINSIC_CODE (decl) = INTRINSIC_NONE;
>> +  DECL_BUILT_IN_CLASS (decl) = NOT_BUILT_IN;
>> +  DECL_FUNCTION_CODE (decl) = BUILT_IN_NONE;
>> +}
>
> It seems wrong that a particular call to a function would change the
> function's properties for all calls, and I don't think there's any
> expectation in the midend that this kind of change might happen.
>
> Why's it needed?  Whatever the reason, I think it needs to be done in a
> different way.
>

There are some D library math functions that are only treated as
built-in during compile-time only.  When it comes to run-time code
generation, we want to call the D library functions, not the C library
(or built-ins).


>> +static tree
>> +expand_intrinsic_bswap (tree callexp)
>> +{
>> +  tree arg = CALL_EXPR_ARG (callexp, 0);
>> +  int argsize = TYPE_PRECISION (TREE_TYPE (arg));
>> +
>> +  /* Which variant of __builtin_bswap* should we call?  */
>> +  built_in_function code = (argsize == 32) ? BUILT_IN_BSWAP32 :
>> +    (argsize == 64) ? BUILT_IN_BSWAP64 : END_BUILTINS;
>> +
>> +  /* Fallback on runtime implementation, which shouldn't happen as the
>> +     argument for bswap() is either 32-bit or 64-bit.  */
>> +  if (code == END_BUILTINS)
>> +    {
>> +      clear_intrinsic_flag (callexp);
>> +      return callexp;
>> +    }
>
> The earlier intrinsics had this too, but I assumed there it was
> because the code was mapping D types to target-dependent ABI types,
> and you couldn't guarantee that long long was a 64-bit type.
> In the above function there's no doubt that 32 and 64 bits are
> the sizes available, so shouldn't this simply assert that
> code != END_BUILTINS?
>

I'll run that through the testsuite, I can only think that it may trip
if integer promotions weren't done as they should.


>> +static tree
>> +expand_intrinsic_sqrt (intrinsic_code intrinsic, tree callexp)
>> +{
>> +  tree arg = CALL_EXPR_ARG (callexp, 0);
>> +  tree type = TYPE_MAIN_VARIANT (TREE_TYPE (arg));
>> +
>> +  /* arg is an integral type - use double precision.  */
>> +  if (INTEGRAL_TYPE_P (type))
>> +    arg = fold_convert (double_type_node, arg);
>> +
>> +  /* Which variant of __builtin_sqrt* should we call?  */
>> +  built_in_function code = (intrinsic == INTRINSIC_SQRT) ? BUILT_IN_SQRT :
>> +    (intrinsic == INTRINSIC_SQRTF) ? BUILT_IN_SQRTF :
>> +    (intrinsic == INTRINSIC_SQRTL) ? BUILT_IN_SQRTL : END_BUILTINS;
>> +
>> +  gcc_assert (code != END_BUILTINS);
>> +  return call_builtin_fn (callexp, code, 1, arg);
>> +}
>
> Converting integers to double precision isn't right for SQRTF and SQRTL.
> But how do such calls reach here?  It looks like the deco strings in the
> function entries provide types for the 3 sqrt intrinsics, is that right?
> Does that translate to a well-typed FUNCTION_DECL, or do the function
> types use some opaque types instead?  If the intrinsic function itself
> is well-typed then an incoming CALLEXP with integer arguments would be
> invalid.
>

As with pow(), I'll have to check by running this through the
testsuite to see what fails and why.

>From memory, the reason is likely some requirement of CTFE, where this
call *must* be const folded at compile-time, which may not happen if
the types are wrong.

>> +/* This is the contribution to the `default_compilers' array in gcc.c
>> +   for the D language.  */
>> +
>> +{".d", "@d", 0, 1, 0 },
>> +{".dd", "@d", 0, 1, 0 },
>> +{".di", "@d", 0, 1, 0 },
>> +{"@d",
>> +  "%{!E:cc1d %i %(cc1_options) %I %{nostdinc*} %{i*} %{I*} %{J*} \
>> +    %{H} %{Hd*} %{Hf*} %{MD:-MD %b.deps} %{MMD:-MMD %b.deps} \
>> +    %{M} %{MM} %{MF*} %{MG} %{MP} %{MQ*} %{MT*} \
>> +    %{X:-Xf %b.json} %{Xf*} \
>> +    %{v} %{!fsyntax-only:%(invoke_as)}}", 0, 1, 0 },
>
> Guess this has been said before, but cc1d seems a strange name,
> since "cc1" is "C Compiler pass 1".
>

It was brought up at the Cauldron.  This predates me, and I've never
had any reason to change it.

I have no problems giving it a better name, I think it's only
Debian/Ubuntu package maintainers that need to be updated on the name
change.

>> +fdeps
>> +D
>> +This switch is deprecated; do not use.
>> +
>> +fdeps=
>> +D Joined RejectNegative
>> +This switch is deprecated; do not use.
>> ...
>> +fintfc
>> +D Alias(H)
>> +; Deprecated in favour of -H
>> +
>> +fintfc-dir=
>> +D Joined RejectNegative Alias(Hd)
>> +; Deprecated in favour of -Hd
>> +
>> +fintfc-file=
>> +D Joined RejectNegative Alias(Hf)
>> +; Deprecated in favour of -Hf
>> ...
>> +ftransition=safe
>> +D Alias(ftransition=dip1000)
>> +; Deprecated in favor of -ftransition=dip1000
>
> Any chance of just removing them?  Would be better not to add new code
> (from GCC's POV) that's already deprecated.
>

I don't think there'd be any tooling that uses these options, so
that's fine by me.


>> +ftransition=all
>> +D RejectNegative
>> +List information on all language changes
>
> All docstrings should end in ".".  This should be tested by adding
> the D frontend to:
>
> foreach cls { "ada" "c" "c++" "fortran" "go" \
>                     "optimizers" "param" "target" "warnings" } {
>
> in gcc.misc-tests/help.exp
>

Ah, good to know!


>> +/* Record information about module initialization, termination,
>> +   unit testing, and thread local storage in the compilation.  */
>> +
>> +struct module_info
>> +{
>> +  vec<tree, va_gc> *ctors;
>> +  vec<tree, va_gc> *dtors;
>> +  vec<tree, va_gc> *ctorgates;
>> +
>> +  vec<tree, va_gc> *sharedctors;
>> +  vec<tree, va_gc> *shareddtors;
>> +  vec<tree, va_gc> *sharedctorgates;
>> +
>> +  vec<tree, va_gc> *unitTests;
>> +};
>> ...
>> +/* Static constructors and destructors (not D `static this').  */
>> +
>> +static vec<tree, va_gc> *static_ctor_list;
>> +static vec<tree, va_gc> *static_dtor_list;
>
> Looks odd to be using GC data structures without marking them.  Think it
> deserves a comment if correct.
>

No, they should be marked.  Will fix that.


>> +Type *
>> +get_object_type (void)
>> +{
>> +  if (ClassDeclaration::object)
>> +    return ClassDeclaration::object->type;
>> +
>> +  ::error ("missing or corrupt object.d");
>
> Why "::"?  Is it needed to avoid shadowing issues?
>

Likely a case of function was converted from a method to a free
function without the body being updated.  I'll correct this.


>> +      /* Strip const modifiers from type before building.  This is done
>> +      to ensure that backend treats i.e: const (T) as a variant of T,
>> +      and not as two distinct types.  */
>
> s/i.e/e.g./
>
> Probably worth running tabify on the sources.  I noticed a couple
> of indents by 8 spaces instead of tabs, but it didn't seem worth
> listing them individually.  You can use contrib/check_GNU_style.{sh,py}
> to find that kind of thing (although it generates a lot of false
> positives for other coding style violations, so take with a pinch
> of salt).
>

Sure, I'll run it though that, thanks for pointing it out.


And thanks again for your time checking this, I'll try to get all
comments addressed in a day or two.

-- 
Iain