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

[Iain Buclaw <ibuclaw at gdcproject dot org>]

On Mon, 15 Oct 2018 at 16:19, David Malcolm <dmalcolm@redhat.com> wrote:
>
> On Tue, 2018-09-18 at 02:33 +0200, Iain Buclaw 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
>
> Hi Iain.  I took at look at this patch, focusing on the diagnostics
> side of things.
>
> These are more suggestions than hard review blockers.
>
> diff --git a/gcc/d/d-attribs.c b/gcc/d/d-attribs.c
> new file mode 100644
> index 00000000000..6c65b8cad9e
> --- /dev/null
> +++ b/gcc/d/d-attribs.c
>
> I believe all new C++ source files are meant to be .cc, rather than .c,
> so this should be d-attribs.cc, rather that d-attribs.c.
>
> [...snip...]
>
> diff --git a/gcc/d/d-codegen.cc b/gcc/d/d-codegen.cc
> new file mode 100644
> index 00000000000..c698890ba07
> --- /dev/null
> +++ b/gcc/d/d-codegen.cc
>
> [...snip...]
>
> +/* Return the GCC location for the D frontend location LOC.   */
> +
> +location_t
> +get_linemap (const Loc& loc)
> +{
>
> I don't like the name "get_linemap", as it suggests to me that it's
> getting the "struct line_map *" for LOC, rather than a location_t.
>
> How about "get_location_t" instead, or "make_location_t"?  The latter
> feels more appropriate, as it's doing non-trivial work.
>

OK.

> +/* Rewrite the format string FORMAT to deal with any format extensions not
> +   supported by pp_format().  The result should be freed by the caller.  */
> +static char *
> +expand_format (const char *format)
>
> Am I right in thinking this is to handle FORMAT strings coming from the
> upstream D frontend, and this has its own formatting conventions?
> Please can the leading comment have example(s) of the format, and what
> it becomes (e.g. the backticks thing).
>
> Maybe adopt a naming convention in the file, to distinguish d format
> strings from pp format strings?  Maybe "d_format" vs "gcc_format"
> ?(though given the verbatim vs !verbatim below, am not sure how
> feasible that is).
>
> Maybe rename this function to expand_d_format??
>
> (Might be nice to add some unittesting of this function via "selftest",
> but that's definitely not a requirement; it's awkward to add right now)
>

Yes, you are right, it's handling format specifiers coming from the
upstream front-end.

The only peculiar convention is the use of backticks to quote parts of
the message (dmd actually does syntax highlighting).

Others in this function just rewrite format specifiers that GCC
diagnostics don't understand.

> +
> +void ATTRIBUTE_GCC_DIAG(2,0)
> +verror (const Loc& loc, const char *format, va_list ap,
> +       const char *p1, const char *p2, const char *)
>
> This one needs a leading comment: what's the deal with P1 and P2?
>

I'll rename them to prefix1 and prefix2 with appropriate comments.

> +
> +void ATTRIBUTE_GCC_DIAG(2,0)
> +vwarning (const Loc& loc, const char *format, va_list ap)
> +{
> +  if (global.params.warnings && !global.gag)
> +    {
> +      /* Warnings don't count if gagged.  */
> +      if (global.params.warnings == 1)
>
> What's the magic "1" value above?  Can it be an enum?  (or is this
> something from the upstream parsing code?)
>

This is from upstream, there are a few places that use numbers instead
of enums.  This is one such place, I've not really given myself any
time to go through them and correct them.  If you feel strongly about
this, I can set aside some time to get this fixed in upstream.

> +  if (global.params.useDeprecated == 0)
> +    verror (loc, format, ap, p1, p2);
> +  else if (global.params.useDeprecated == 2 && !global.gag)
>
> Can these "0" and "2" values be enums?
>

Same as above, I'll push this upstream and trickle it down here.

>
> Hope this is constructive

Thanks
-- 
Iain