branch master updated: * tp/Texinfo/XS/main/DocumentXS.xs (document_errors): warn if a document C data is not found.

[Patrice Dumas]

This is an automated email from the git hooks/post-receive script.

pertusus pushed a commit to branch master
in repository texinfo.

The following commit(s) were added to refs/heads/master by this push:
     new 91e6aba21e * tp/Texinfo/XS/main/DocumentXS.xs (document_errors): warn 
if a document C data is not found.
91e6aba21e is described below

commit 91e6aba21efd0e91d234b92021e2e9032260cda1
Author: Patrice Dumas <pertusus@free.fr>
AuthorDate: Sun May 26 22:03:45 2024 +0200

    * tp/Texinfo/XS/main/DocumentXS.xs (document_errors): warn if a
    document C data is not found.
    
    * tp/Texinfo/XS/main/build_perl_info.c (add_formatted_error_messages):
    remove error messages for situations that should not happen.
---
 ChangeLog                            |  8 ++++++++
 tp/Texinfo/ParserNonXS.pm            |  1 -
 tp/Texinfo/XS/main/DocumentXS.xs     | 12 +++++++++++-
 tp/Texinfo/XS/main/build_perl_info.c | 34 +++++++++++++++++++---------------
 tp/Texinfo/XS/parsetexi/Parsetexi.pm |  1 -
 5 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 9bb0655c40..b4e444360d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-05-26  Patrice Dumas  <pertusus@free.fr>
+
+       * tp/Texinfo/XS/main/DocumentXS.xs (document_errors): warn if a
+       document C data is not found.
+
+       * tp/Texinfo/XS/main/build_perl_info.c (add_formatted_error_messages):
+       remove error messages for situations that should not happen.
+
 2024-05-26  Patrice Dumas  <pertusus@free.fr>
 
        * tp/t/test_parse_texi_line.t: change in variable names, test the
diff --git a/tp/Texinfo/ParserNonXS.pm b/tp/Texinfo/ParserNonXS.pm
index 03ce0246c0..6180c48838 100644
--- a/tp/Texinfo/ParserNonXS.pm
+++ b/tp/Texinfo/ParserNonXS.pm
@@ -578,7 +578,6 @@ sub parser(;$)
   if (defined($conf)) {
     foreach my $key (keys(%$conf)) {
       if (exists($Texinfo::Common::parser_document_parsing_options{$key})) {
-        # we keep registrar instead of copying on purpose, to reuse the object
         if (ref($conf->{$key})) {
           $parser_conf->{$key} = dclone($conf->{$key});
         } else {
diff --git a/tp/Texinfo/XS/main/DocumentXS.xs b/tp/Texinfo/XS/main/DocumentXS.xs
index 007d4c2a14..6b7d937bec 100644
--- a/tp/Texinfo/XS/main/DocumentXS.xs
+++ b/tp/Texinfo/XS/main/DocumentXS.xs
@@ -114,7 +114,12 @@ document_errors (SV *document_in)
         SV *error_nrs_sv = 0;
         ERROR_MESSAGE_LIST *error_messages = 0;
      PPCODE:
-        document = get_sv_document_document (document_in, 0);
+        /* if XS is used, a document should be found.  It could
+           also have been possible to abort if a document is not
+           found.
+         */
+        document = get_sv_document_document (document_in,
+                                             "document_errors");
         if (document)
           error_messages = &document->error_messages;
 
@@ -123,6 +128,11 @@ document_errors (SV *document_in)
                                   &error_nrs_sv);
         clear_error_message_list (error_messages);
 
+        /* NOTE this is incorrect, as the callers do not expect
+           undef errors_warnings_sv.  This should not happen, however,
+           as registrar objects are always associated to documents 
+           and the corresponding array reference should always be found
+         */
         if (!errors_warnings_sv)
           errors_warnings_sv = newSV (0);
         else
diff --git a/tp/Texinfo/XS/main/build_perl_info.c 
b/tp/Texinfo/XS/main/build_perl_info.c
index 042256ee21..7977ebe240 100644
--- a/tp/Texinfo/XS/main/build_perl_info.c
+++ b/tp/Texinfo/XS/main/build_perl_info.c
@@ -1230,7 +1230,7 @@ build_errors (const ERROR_MESSAGE *error_list, size_t 
error_number)
    NOTE probably not useful for converters as errors need to be passed
    explicitely both from Perl and XS and are added at that point.
 
-   Also return $report->{'errors_warnings'} in ERRORS_WARNINGS_OUT and
+   Returns $report->{'errors_warnings'} in ERRORS_WARNINGS_OUT and
    $report->{'error_nrs'} in ERRORS_NRS_OUT, even if ERROR_MESSAGES is
    0, to avoid the need to fetch them from report_hv if calling code
    is interested in those SV.
@@ -1246,16 +1246,9 @@ add_formatted_error_messages (const ERROR_MESSAGE_LIST 
*error_messages,
 
   dTHX;
 
-
   *errors_warnings_out = 0;
   *error_nrs_out = 0;
 
-  if (!report_hv)
-    {
-      fprintf (stderr, "add_formatted_error_messages: BUG: no perl report\n");
-      return;
-    }
-
   errors_warnings_sv = hv_fetch (report_hv, "errors_warnings",
                                  strlen ("errors_warnings"), 0);
 
@@ -1274,11 +1267,10 @@ add_formatted_error_messages (const ERROR_MESSAGE_LIST 
*error_messages,
 
       if (!error_messages)
         {
-          /* TODO if this message appears in output, it should probably
-             be removed, as this situation is allowed from DocumentXS.xs
-             document_errors */
-          fprintf (stderr,
-               "add_formatted_error_messages: NOTE: no error_messages\n");
+          /* See the comment before pass_errors_to_registrar, this probably
+             cannot happen.  We do not warn here, there should already
+             be other warnings as it means that no XS document was found.
+           */
           return;
         }
       else
@@ -1319,8 +1311,13 @@ add_formatted_error_messages (const ERROR_MESSAGE_LIST 
*error_messages,
     }
 }
 
-/* ERROR_MESSAGES can be 0, in that case the function is used to get
-   the perl references but they are not modified */
+/* ERROR_MESSAGES could be 0, in that case the function is used to get
+   the perl references but they are not modified.
+   Error messages set to 0, however, cannot happen in practice, as it cannot
+   happen when called through pass_document_parser_errors_to_registrar for
+   parser errors, and for document errors it would mean no XS document found,
+   which cannot happen right now and would probably trigger many warnings.
+ */
 SV *
 pass_errors_to_registrar (const ERROR_MESSAGE_LIST *error_messages,
                           SV *object_sv,
@@ -1336,6 +1333,10 @@ pass_errors_to_registrar (const ERROR_MESSAGE_LIST 
*error_messages,
 
   registrar_sv = hv_fetch (object_hv, registrar_key,
                            strlen (registrar_key), 0);
+  /* A registrar is systematically added to document by parsers, so the
+     condition should always be true.  errors_warnings_out
+     should always be set and it is a good thing because
+     errors_warnings_out is not supposed to be undef */
   if (registrar_sv && SvOK (*registrar_sv))
     {
       HV *report_hv = (HV *) SvRV (*registrar_sv);
@@ -1360,8 +1361,11 @@ pass_document_parser_errors_to_registrar (int 
document_descriptor,
 
   document = retrieve_document (document_descriptor);
 
+  /* This cannot happen, the function is called on a document that
+     was just registered
   if (!document)
     return;
+   */
 
   pass_errors_to_registrar (&document->parser_error_messages, parser_sv,
                             &errors_warnings_sv, &error_nrs_sv);
diff --git a/tp/Texinfo/XS/parsetexi/Parsetexi.pm 
b/tp/Texinfo/XS/parsetexi/Parsetexi.pm
index a96bbe0a04..93a790d4a6 100644
--- a/tp/Texinfo/XS/parsetexi/Parsetexi.pm
+++ b/tp/Texinfo/XS/parsetexi/Parsetexi.pm
@@ -71,7 +71,6 @@ sub parser (;$)
   if (defined($conf)) {
     foreach my $key (keys(%$conf)) {
       if (exists($Texinfo::Common::parser_document_parsing_options{$key})) {
-        # we keep registrar instead of copying on purpose, to reuse the object
         if (ref($conf->{$key})) {
           $parser_conf->{$key} = dclone($conf->{$key});
         } else {