On Sat, 20 Oct 2018 at 11:03, Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Iain Buclaw <ibuc...@gdcproject.org> writes:
> > On 14 October 2018 at 17:29, Richard Sandiford
> > <rdsandif...@googlemail.com> wrote:
> >> [Sorry if this turns out to do be a dup]
> >>
> >> Iain Buclaw <ibuc...@gdcproject.org> writes:
> >>> +/* 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.
>
> OK. In that case I think you want to use diagnostic_set_info_translated
> instead of diagnostic_set_info, so that we don't try to translate things
> that aren't meant to be translated. Also it would be good to reword the
> comment above the function, since "where the message FORMAT has not yet
> been translated by the gcc diagnostic routines" made it sound like these
> messages were supposed to be translated at some point, which is where the
> confusion started. :-)
>
OK, done.
> >>> +/* 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.
>
> If it's for UTF-16 and UTF-32 then I think it's fine as-is (if you keep it).
> Was just asking in case.
>
OK.
> >>> +/* 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.
>
> But my point was more that the function seems to have the effect of
> returning 0 all the time, so wouldn't it be better to get rid of the
> code before the final "return 0"? Or is there a case I missed?
>
Yes, it only ever returns 0. I'll just remove the other parts, I have
it in commit history should I ever attempt to support this proper.
Regards
--
Iain
