Hi David.
> On Sun, 2025-11-30 at 02:06 +0100, Jose E. Marchesi wrote: >> This commit adds the diagnostics infrastructure for the Algol 68 >> front-end. > > Hi Jose, congrats on getting all this merged. Thanks! :) > A few comments inline below: > >> >> Signed-off-by: Jose E. Marchesi <[email protected]> >> Co-authored-by: Marcel van der Veer <[email protected]> >> >> gcc/ChangeLog >> >> * algol68/a68-diagnostics.cc: New file. >> --- >> gcc/algol68/a68-diagnostics.cc | 381 >> +++++++++++++++++++++++++++++++++ >> 1 file changed, 381 insertions(+) >> create mode 100644 gcc/algol68/a68-diagnostics.cc >> >> diff --git a/gcc/algol68/a68-diagnostics.cc b/gcc/algol68/a68- >> diagnostics.cc >> new file mode 100644 >> index 00000000000..0c25da2a21f >> --- /dev/null >> +++ b/gcc/algol68/a68-diagnostics.cc >> @@ -0,0 +1,381 @@ >> +/* Error and warning routines. >> + Copyright (C) 2001-2023 J. Marcel van der Veer. >> + Copyright (C) 2025 Jose E. Marchesi. >> + >> + Original implementation by J. Marcel van der Veer. >> + Adapted and expanded for GCC by Jose E. Marchesi. >> + >> + GCC is free software; you can redistribute it and/or modify it >> + under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3, or (at your >> option) >> + any later version. >> + >> + GCC is distributed in the hope that it will be useful, but >> WITHOUT >> + ANY WARRANTY; without even the implied warranty of >> MERCHANTABILITY >> + or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public >> + License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with GCC; see the file COPYING3. If not see >> + <http://www.gnu.org/licenses/>. */ >> + >> +#define INCLUDE_MEMORY >> +#include "config.h" >> +#include "system.h" >> +#include "coretypes.h" >> +#include "diagnostic.h" >> + >> +#include "a68.h" >> + >> +/* >> + * Error handling routines. >> + */ >> + >> +#define TABULATE(n) (8 * (n / 8 + 1) - n) >> + >> +/* Severities handled by the DIAGNOSTIC function defined below. */ >> + >> +#define A68_ERROR 0 >> +#define A68_WARNING 1 >> +#define A68_FATAL 2 >> +#define A68_SCAN_ERROR 3 >> +#define A68_INFORM 4 >> + >> +/* Give a diagnostic message. */ >> + >> +#if __GNUC__ >= 10 >> +#pragma GCC diagnostic ignored "-Wsuggest-attribute=format" >> +#endif >> + >> +static bool >> +diagnostic (int sev, int opt, >> + NODE_T *p, >> + LINE_T *line, >> + char *pos, >> + const char *loc_str, va_list args) >> +{ >> + int res = 0; >> + MOID_T *moid = NO_MOID; >> + const char *t = loc_str; >> + obstack b; >> + >> + /* >> + * Synthesize diagnostic message. >> + * >> + * Legend for special symbols: >> + * * as first character, copy rest of string literally >> + * @ AST node >> + * A AST node attribute >> + * B keyword >> + * C context >> + * L line number >> + * M moid - if error mode return without giving a message >> + * O moid - operand >> + * S quoted symbol, when possible with typographical display >> features >> + * X expected attribute >> + * Y string literal. >> + * Z quoted string. */ > > Out of curiosity, is it possible to implement all of this via a > pp_format_decoder callback, rather than generating a format string for > use by pp_format? > > [...snip...] > >> + >> + switch (att) >> + { >> + case NO_SORT: sort = "this"; break; >> + case SOFT: sort = "a soft"; break; >> + case WEAK: sort = "a weak"; break; >> + case MEEK: sort = "a meek"; break; >> + case FIRM: sort = "a meek"; break; > > Should "sort" be "a firm" in "case FIRM:" above? (copy-and-paste > error?) Will fix. >> + case STRONG: sort = "a strong"; break; >> + default: >> + gcc_unreachable (); >> + } >> + >> + obstack_grow (&b, sort, strlen (sort)); >> + } >> + else if (t[0] == 'L') >> + { >> + LINE_T *a = va_arg (args, LINE_T *); >> + gcc_assert (a != NO_LINE); >> + if (NUMBER (a) == 0) >> + obstack_grow (&b, "in standard environment", >> + strlen ("in standard environment")); >> + else if (p != NO_NODE && NUMBER (a) == LINE_NUMBER (p)) >> + obstack_grow (&b, "in this line", strlen ("in this >> line")); >> + else >> + { >> + char d[10]; >> + if (snprintf (d, 10, "in line %d", NUMBER (a)) < 0) >> + gcc_unreachable (); > > If I'm counting correctly, "in line " is 8 chars, so leaving only one > char for the number, and one for the nul-terminator, which suggests > NUMBER (a) can only be a single digit. Is that correct, or should the > size be increased from 10? (used in both d and the snprintf call). Urgh yeah, will fix by increasing the buffer size. > >> + obstack_grow (&b, d, strlen (d)); >> + } > > [...snip...] > >> + obstack_grow (&b, "UNION (VOID, ..)", strlen ("UNION >> (VOID, ..)")); > > There's a lot of > obstack_grow (&b, str, strlen (str); > in this code where we hope that "UNION (VOID, ..)" is the same string > as "UNION (VOID, ..)". How about a helper function, something like: > > obstack_append_str (&b, str); > > (or a macro, if you're trying to avoid the runtime-computation of > strlen, but this is diagnostic code and so almost certainly not a hot- > spot in the profile). Yeah, I thought about that while I was writing the code, but then somehow forgot about it or got distracted with something else :) Will fix. Thank you for your comments! > > [...snip...] > > Hope this is constructive > Dave
