On Tue, Jun 11, 2019 at 10:30:59AM +0100, Mark Eggleston wrote:
> Jim MacArthur <jim.macart...@codethink.co.uk>
> Mark Eggleston <mark.eggles...@codethink.com>
Two spaces before < instead of one.
This is not a patch review, just comments:
> This has to be written in a slightly verbose manner because GCC 7
> defaults to building with -Werror=implicit-fallthrough which prevents
> us from just falling through to the default: case.
That is not true, one can fall through just fine, just there needs to be a
comment or attribute or builtin that says so.
> --- a/gcc/fortran/io.c
> +++ b/gcc/fortran/io.c
> @@ -598,6 +598,7 @@ check_format (bool is_input)
> {
> const char *posint_required = _("Positive width required");
> const char *nonneg_required = _("Nonnegative width required");
> + const char *missing_item = _("Missing item");
> const char *unexpected_element = _("Unexpected element %qc in format "
> "string at %L");
> const char *unexpected_end = _("Unexpected end of format string");
> @@ -756,6 +757,16 @@ format_item_1:
> error = unexpected_end;
> goto syntax;
>
> + case FMT_RPAREN:
> + /* Oracle allows a blank format item. */
> + if (flag_dec_blank_format_item)
> + goto finished;
> + else
> + {
> + error = missing_item;
> + goto syntax;
> + }
So, if you want to fall thru, just do:
case FMT_RPAREN:
if (flag_dec_blank_format_item)
goto finished;
/* FALLTHRU */
> +
> default:
> error = unexpected_element;
> goto syntax;
and that is it. Not sure I'd mention the Oracle fortran compiler there,
either it is common to other DEC based compilers too (DEC fortran, ifort,
...) and then the comment makes no sense, or it might not be best to call
the flag -fdec-whatever.
Furthermore, even if you want to have a _("Missing item"), you should write
it as error = _("Missing item");, not the way it is written, as that way it
is inefficient at compile time.
The rest is just a general comment on the preexisting code.
Using a bunch of const char *whatever = _("...");
at the start of function is undesirable, it means any time this function is
called, even in the likely case there is no error, all those strings need to
be translated. It would be better to e.g. replace all _("...") in that
function with G_("...") (i.e. mark for translation, but don't translate),
and only when actually using that translate:
if (error == unexpected_element)
gfc_error (error, error_element, &format_locus);
else
gfc_error ("%s in format string at %L", error, &format_locus);
The first case is translated already by gfc_error, the second one would need
_(error) instead of error (but is generally wrong anyway, because you are
constructing a diagnostics from two pieces which might not be ok for
translations. So, likely you want to append " in format string at %L" to
all the error string literals inside of G_("...") and just pass error as
first argument to gfc_error.
Jakub
