feat: tagfiles generation for cross-referencing in doxygen format

[fpelliccioni]

Fixed #650

[alandefreitas]

The logic for the Writer looks great. I think the only issue is where it would be called from.

[alandefreitas]

The problem CI has been fixed. Could you rebase this?

[alandefreitas]

However, we will still have a problem with the USR from Krystian's commit blocking CI.

I want to start by comparing the XML in the artifacts. Do you have the result for Boost.URL multipage? That's the one we can't risk breaking because it's already in production. It has to be compatible with the existing one.

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[alandefreitas]

It fails with

assertion failed: n <= indent_.size() on line 203 in src/lib/Gen/xml/XMLTags.cpp

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[alandefreitas]

The implementation of the tagfile write is excellent.

CI failed after I rebased. That was a good thing because it helped me catch a bug.

So, the reason CI is failing is trivial. The TestRunner calls if(auto err = gen_->buildOneString(generatedDocs, **corpus)), which calls Generator::buildOneString, where you changed the call to buildOne to buildOne(ss, corpus, "");. We could fix CI by just adapting AdocGenerator::buildOne to not generate the tagfile when this string is empty. However, this doesn't address the deeper issue here.

The real issue causing all of that is the API design. Your new fileName parameter in Generator::buildOne is breaking the contract of this abstract class.

1. First of all, the new parameter is documented as @param fileName The file name to use for the output., but that's not what the implementation is doing. The filename is the filename for the tagfile, which is unrelated to the filename of the output. The parameter isn't even used in the XMLGenerator. It's even more confusing because fileName is not the filename of the tagfile but the basename to which "tag.xml" will be appended to form the tagfile filename.
2. Most importantly, the contract of Generator::buildOne is that it renders the documentation to any output stream regardless of the filename because (i) there's not even a filename for most output types and (ii) if there were a file for each stream, the stream would become redundant. For instance, tests use buildOneString, which outputs the result to a string.

This bug was already there, but CI only made it evident. It worked before rebasing because Generator::buildOneString wasn't being covered with the AdocGenerator.

Because of how Generator expects the output to be decoupled from any filename and buildOne is based on streaming only that result to the output, the Generator should have received new virtual functions to buildTagfile(std::ostream& os, Corpus const& corpus) or something instead of buildOne trying to fill this role.

If this is hard to achieve because we can't decouple the logic of buildOne and buildTagfile, a second-best would be buildOne(std::ostream& os, Corpus const& corpus, std::ostream& tagFileOs) instead of buildOne(std::ostream& os, Corpus const& corpus, std::string_view fileName) and a new overload in Generator without std::ostream& tagFileOs which would call buildOne with a noop std::ostream for the tagFileOs.

This fixes the underlying problem and the original issue with buildOneString that's breaking CI.

[fpelliccioni]

The implementation of the tagfile write is excellent.

CI failed after I rebased. That was a good thing because it helped me catch a bug.

So, the reason CI is failing is trivial. The TestRunner calls if(auto err = gen_->buildOneString(generatedDocs, **corpus)), which calls Generator::buildOneString, where you changed the call to buildOne to buildOne(ss, corpus, "");. We could fix CI by just adapting AdocGenerator::buildOne to not generate the tagfile when this string is empty. However, this doesn't address the deeper issue here.

The real issue causing all of that is the API design. Your new fileName parameter in Generator::buildOne is breaking the contract of this abstract class.

1. First of all, the new parameter is documented as @param fileName The file name to use for the output., but that's not what the implementation is doing. The filename is the filename for the tagfile, which is unrelated to the filename of the output. The parameter isn't even used in the XMLGenerator. It's even more confusing because fileName is not the filename of the tagfile but the basename to which "tag.xml" will be appended to form the tagfile filename.
2. Most importantly, the contract of Generator::buildOne is that it renders the documentation to any output stream regardless of the filename because (i) there's not even a filename for most output types and (ii) if there were a file for each stream, the stream would become redundant. For instance, tests use buildOneString, which outputs the result to a string.

This bug was already there, but CI only made it evident. It worked before rebasing because Generator::buildOneString wasn't being covered with the AdocGenerator.

Because of how Generator expects the output to be decoupled from any filename and buildOne is based on streaming only that result to the output, the Generator should have received new virtual functions to buildTagfile(std::ostream& os, Corpus const& corpus) or something instead of buildOne trying to fill this role.

If this is hard to achieve because we can't decouple the logic of buildOne and buildTagfile, a second-best would be buildOne(std::ostream& os, Corpus const& corpus, std::ostream& tagFileOs) instead of buildOne(std::ostream& os, Corpus const& corpus, std::string_view fileName) and a new overload in Generator without std::ostream& tagFileOs which would call buildOne with a noop std::ostream for the tagFileOs.

This fixes the underlying problem and the original issue with buildOneString that's breaking CI.

This issue is addressed. I’ll let you know once CI passes so you can review, and if it’s good to go, feel free to merge.

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[fpelliccioni]

@alandefreitas done

[alandefreitas]

This solution is still not quite there yet.

[alandefreitas]

Do we need this function? Can't the caller use traverse as usual, evaluate the predicate in the callable f, and do nothing if the predicate is false? It seems like less cognitive bandwidth—one less function to remember and maintain.

[alandefreitas]

My impression is that this solution expands the problem rather than solves it. Now, not only we expect a std::string tagFileName; (which breaks the contract of the class) but we expect a redundant std::ostream& tagFileOs; that needs to be handled and maintained.

I don't know if you're familiar with SOLID, but maybe have a look at https://medium.com/@oleksandra_shershen/solid-principles-implementation-and-examples-in-c-99f0d7e3e868.

The original problem is that expecting std::string tagFileName as a parameter breaks the contract of the parent class because the single responsibility of buildOne should be streaming the documentation results to any output. In other words, ideally, the parent Generator class shouldn't mention anything related to tagfiles because being able to generate tagfiles is not a property of all generators. If only some generators have this functionality, ideally, there should be an intermediary class (say GeneratorThatAlsoGeneratesTagfiles to avoid bikeshedding for now) between Generator and the generators that so have tagfiles functionality (AdocGenerator and HTMLGenerator). "GeneratorThatAlsoGeneratesTagfiles" would inherit from Generator and define the common API for generating tagfiles (which might have many similarities for all generators). AdocGenerator and HTMLGenerator would then inherit from "GeneratorThatAlsoGeneratesTagfiles" and only implement whatever is different for Adoc and HTML, which I don't expect to be much because only the file extension of the filenames should be different.

The solution I proposed is a buildTagfile(std::ostream& os, Corpus const& corpus), ideally in this new intermediary base class with the common logic for tagfiles. If we really don't want an intermediary class for "convenience", then this buildTagfile(std::ostream& os, Corpus const& corpus) function could go directly in the generator and do nothing when the Generator cannot generate tagfiles.

If the logic for tag files can't be decoupled from the logic for buildOne, maybe we could consider more complex solutions because it's impossible to implement its single responsibility. But it's clear that tagfiles can be implemented separately because they're basically an XML generator with a different schema.

[alandefreitas]

We need an option here, such as:

https://www.doxygen.nl/manual/config.html#cfg_generate_tagfile

Its default value can be "reference.tag.xml" or whatever and tagfiles won't be generated when it's empty.

[alandefreitas]

Completely independent behavior. It's not even using the domCorpus that includes the reference file names. It shouldn't be made dependent on build for "convenience".