Subject: Re: bug#75755: feature/igc: Missing IGC_CHECK_RES?

From: Pip Cet <[email-masked]>
Date: Thu, 23 Jan 2025 14:34:56 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> From: Stefan Kangas <[email-masked]>
>> Date: Wed, 22 Jan 2025 13:52:07 -0800
>> Cc: Gerd Möllmann <[email-masked]>,
>> 	[email-masked], [email-masked], Helmut Eller <[email-masked]>,
>> 	Eli Zaretskii <[email-masked]>, Andrea Corallo <[email-masked]>
>>
>> Pip Cet <[email-masked]> writes:
>>
>> > "Stefan Kangas" <[email-masked]> writes:
>> >
>> >> Gerd Möllmann <[email-masked]> writes:
>> >>
>> >>> Pip Cet <[email-masked]> writes:
>> >>>
>> >>>> "Pip Cet" <[email-masked]> writes:
>> >>>>
>> >>>> My preference would be to add this code before result_string:
>> >>>>
>> >>>> /* Define a named enumeration containing all cases that the integer type
>> >>>>    mps_res_t is known to cover.  */
>> >>>>
>> >>>> enum mps_res_enum
>> >>>> {
>> >>>>   _mps_RES_ENUM (RES_CASE, MPS_RES_)
>> >>>> };
>> >>>> #undef RES_CASE
>> >>>
>> >>> That would be nice, indeed.
>> >>
>> >> Can we rely on this or is it an internal API?
>> >
>> > It's an internal API.  The header has this to say:
>> >
>> >  * .naming.internal: Any identifier beginning with an underscore is for
>> >  * internal use within the interface and may change or be withdrawn without
>> >  * warning.
>> >
>> > I don't think that it's always horrible to rely on internal APIs,
>> > though.  Maybe we can put it inside "#ifdef _mps_RES_ENUM", with a
>> > comment explaining that if in some distant future there are compilation
>> > errors because _mps_RES_ENUM is defined differently, the #ifdef block
>> > can safely be omitted?
>>
>> AFAICT, being non-exhaustive risks that people will get suboptimal
>> errors, and being exhaustive means that we can get optimal errors on
>> master sooner.
>>
>> OTOH, the situation I'm concerned about is if someone is trying to build
>> an old Emacs tarball with the latest MPS.  If the internal identifier
>> disappears, or its interface changes, the build will be broken.  The MPS
>> developers warn that such a change could happen "without warning".
>>
>> Are the benefits of using it large enough to be worth the risks?  I'm
>> currently leaning towards "no", but it's possible that I'm missing
>> something.
>
> IMO, the single use of this macro is not worth of the complications
> due to relying on an internal enumeration macro.  The single function
> which uses this is a testing tool, which on top of that warns in its
> doc string not to use it.  Introducing significant complexity and
> breakage potential into Emacs due to this is IMO a tail wagging the
> dog.

I agree with this paragraph; it's not worth it, let's drop the code.

> So I suggest that we define our own enumeration.  We could copy some

Why?

mps.h does define an enumeration, it just fails to give it a name (we'd
use it then), or use it in a typedef (we could use it then), and we
can't cast to typeof (MPS_RES_OK) because, well, that's a gcc bug.

(Yes, C defines typeof(ENUM_VALUE) to be equivalent to int; but the C
standard also doesn't prohibit the compiler from remembering where that
"int" came from, that it refers to the underlying type of a specific
enum, and adding extra warnings based on this memory)

So my preference is to remove the code and cast to typeof (MPS_RES_OK),
briefly mentioning in a comment that this approach does not work yet,
but might work in the future.

(I still think the GCC people should warn if a switch statement contains
labels for some but not all elements of an enum, even if the switch
value is cast to int.  It's the labels that matter, not the matched
value.  Works, see patch.  I assume GCC folks will disagree.)

> information from the MPS headers if needed (or not: I'm not sure we
> should be wedded to their strings in this case).  Yes, that would mean

We should use our strings, which Stefan Kangas provided (and which are
in use by the current code).

> if they add or remove enumeration values, we will have some
              ^^^^^^^^^

If they remove enumeration values, that's a breaking change, and the
build will break, as it should. (Warning about non-breaking changes is
better than breaking the build, but breaking the build is still better
than silently accepting a situation which is likely to result in runtime
errors).

> maintenance work on our hands, but it doesn't seem a significant
> problem for an internal function that almost no one should be using.

Just so we don't forget this isn't about this specific case only:

We've yet to add the TSQueryErrorLanguage case to treesit.c because
there was no compile-time warning.  It's a lot more work than it would
have been if the warning had been present.

diff --git a/src/treesit.c b/src/treesit.c
index d4090b949ea..7f34ef085dd 100644
--- a/src/treesit.c
+++ b/src/treesit.c
@@ -1446,9 +1446,13 @@ treesit_query_error_to_string (TSQueryError error)
       return "Capture error at";
     case TSQueryErrorStructure:
       return "Structure error at";
-    default:
-      return "Unknown error";
+      /* FIXME: what's a "language error"?  Do all versions of
+	 treesitter we support define this enumeration value?
+    case TSQueryErrorLanguage:
+      return "Language error at";
+      */
     }
+  return "Unknown error";
 }
 
 static Lisp_Object

In the case of treesit.c, the problem would have been printed by
-Wswitch-enum, or by writing treesit_query_error_to_string more
carefully.

Switching to -Wswitch-enum would be good, but cause a number of warnings
for code which is perfectly acceptable because it uses a local or
logically complete enum instead of one which is subject to unexpected
changes.

And, yes, two or three places would require #pragma's to avoid the
warning because an external enum we cannot control or establish the
potential values of in a preprocessor macro might change under us and
otherwise cause noise.

I'm volunteering to do this, except for bidi.c; I'll put a #pragma in to
avoid the warning, and then someone who understands the code better can
decide whether to make the code comply to -Wswitch-enum or not.  Let me
know if this is desirable and I'll send a patch (which will be large
because things like the switch (XTYPE (elt)) in xdisp.c change
indentation levels).

Pip