Re: Further reducing special case code for XS structuring

[Patrice Dumas]

On Thu, Apr 04, 2024 at 10:23:13PM +0100, Gavin Smith wrote:
> I am working on changes for this, but it still doesn't seem right to me,
> in the case of XS structuring being on, and XS conversion being off.
> 
> As of commit 5f6800edc (2024-04-03 13:29:19 +0100), in texi2any.pl the
> tree is retrieved on line 1526:
> 
>       goto NEXT;
>     }
>   
>     my $tree = $document->tree(1);
>     if ($tree_transformations{'fill_gaps_in_sectioning'}) {
>       Texinfo::Transformations::fill_gaps_in_sectioning($tree);
>     }
> 
> 
> The changes involved changes to this part of the code:
> 
> 
> @@ -1523,7 +1529,14 @@ while(@input_files) {
>      goto NEXT;
>    }
>  
> -  my $tree = $document->tree(1);
> +  my $tree;
> +  if (!$XS_convert) {
> +    $tree = $document->tree();
> +  } else {
> +    # The argument prevents the tree being built as a Perl structure in
> +    # case XS code is being used all the way through to conversion.
> +    $tree = $document->tree(1);
> +  }
>    if ($tree_transformations{'fill_gaps_in_sectioning'}) {
>      Texinfo::Transformations::fill_gaps_in_sectioning($tree);
>    }
> 
> 
> However, this is before several transformations take place on the
> structure.  So if XS structuring is on, these transformations will (or
> should) take place on the C tree, and the building of the Perl tree at
> this stage seems needless.

Indeed, my advice was probably correct only for your previous patch
where $XS_structuring was always set.

> (It also seems a bit confusing that some transformations use a "tree"
> object and some use a "document" object.)

I pass a document when some information from the document available
through an accessor is needed.  If no such information is needed,
and the tree only is needed passing a document would not be natural.
But it could still be better for consistency of the interface.  I do not
have an opinion on what is best.

> So I thought it should depend on "XS structuring" being on, rather than
> "XS conversion":
> 
>     my $tree;
>     if (!$XS_structuring) {
>       $tree = $document->tree();
>     } else {
>       # The argument prevents the tree being built as a Perl structure at
>       # this stage; only a "handle" is returned.
>       $tree = $document->tree(1);
>     }
> 
> Does this look right?

It looks right to me.

I have not checked, but it could possibly be shorter, though maybe less explicit
and instead a if, call only:
  $tree = $document->tree($XS_structuring);

> (Admittedly, this is still special-casing for XS, which I was trying to
> eliminate, although it's still simpler overall, in my opinion.)

My feeling is that the special casing cannot be eliminated, it can only
be transferred to a stage or another.

> Then it should be possible to simplify parse_texi_file in Parsetexi.pm
> not to take the extra argument ($no_build).  This would be good, as it
> would match parse_texi_file in ParserNonXS.pm, which doesn't have the
> extra argument.  I feel that XS and pure Perl functions should present
> the same interface as far as is practicable.  (Anyone reading a call
> to parse_texi_file in the Perl code might find it harder to delve into
> the XS code to understand what the extra argument does.  There's an
> immediate barrier in working out that Texinfo::Parser::parse_texi_file
> is actually implemented in Texinfo/XS/parsetexi/Parsetexi.pm - not that
> this is avoidable, of course.  Then they may have to understand the
> role of *.xs files in defining XS subroutines, like 'get_document' or
> 'build_document' if they want to understand what these do.)

I do not think that having the same interface for XS and pure Perl is
important.  It could even make clearer what is going on in XS.  What is
important is to have a code that is easy to understand as a whole in my
opinion.

> The current state of the change is below.  I'd also plan on reviewing
> the other parse_texi_* functions in Parsetexi.pm (such as parse_texi_piece)
> to see if the $no_build and $no_store arguments are necessary.

I expect no_build not to be relevant anymore, as with your changes
building Perl structures is never done by the Parser if the XS Parser is
used, building Perl structures is always done later on.

I am not sure that no_store is used, but I think that it should be
considered completely separately from no_build and the on-demand Perl
structures build, as it is a completely different issue.

> (I had to change t/protect_character_in_texinfo.t to keep the call to
> Texinfo::Document::rebuild_tree working - this is the only place in the
> code base this function is called.)

I would actually have preferred to remove all the calls to that
function, but didn't manage to remove this one (I do not remember
exactly why, maybe because there are only $tree and no $document
available or something like that).


Your changes look right to me.

-- 
Pat