Page MenuHomePhabricator

[clang-doc] Adds HTML generator
AbandonedPublic

Authored by DiegoAstiazaran on Jun 11 2019, 6:56 PM.

Details

Summary

Implement a simple HTML generator based on the existing Markdown generator.
Basic HTML tags are included: h, p.
HTML templates will be used in the future instead of generating each of the HTML tags individually.

Diff Detail

Event Timeline

DiegoAstiazaran edited the summary of this revision. (Show Details)Jun 11 2019, 6:56 PM
juliehockett added inline comments.Jun 21 2019, 10:05 AM
clang-tools-extra/clang-doc/HTMLGenerator.cpp
88

You shouldn't be using writeLine here, since it wraps the tag in an extra <p> element (which is okay for all the other pieces, but is weird on a specified HTML element.

90

Same as above

111

The pipes don't mean anything in HTML, so you can remove them.

clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
215–219

For the sake of vaguely valid HTML in the test output, it'd be nice if we had an end tag for the <li> element as well.

288

Should the whole comment be wrapped in a <p> element?

I got to the 'genHTML' functions and I decided that this general approach is not great.

What if we have a baby internal representation of an HTML tree, generate instances of that, and then implement a render method on that (preferably one that handles indentation or would have the ability to do so soon).

I also think we could split this up and support only a subset of the comment types at first to reduce the review load.

clang-tools-extra/clang-doc/Generators.cpp
60–71

This seems to be displaying a list in a particular way. Mind adding a comment about where this is used?

65

You can just use &R != Refs.begin() or &R != &Refs.front() and then you don't have to keep any local state and its clear when that condition would be true/false.

clang-tools-extra/clang-doc/HTMLGenerator.cpp
26

This can also be a Twine

34–36

I'm not familiar with HTML. What does this '<p>' do?

67

Instead of repeating code add an "||" case to the above.

68–69

Instead of making a local std::string you can first write the emphasis then decide wheater or not to output the direction, and then output the newlines unconditionally.

73–78

Dedup these.

82–85

I don't see the need for an intermediete ostream.

OS << "<" << I.Name;
for (...)
  OS << ...;
writeLine(I.SelfClosing ? ..., OS);

would work fine and be more efficient.

87

This can be a StringRef

107–108

This is a generally bad pattern.

DiegoAstiazaran marked 4 inline comments as done.
DiegoAstiazaran edited the summary of this revision. (Show Details)

Removed rendering of some CommentInfo kinds to reduce review load.
Removed all <br>, <em>, and <b> tags; they are not necessary for the first version of the HTML generator.
Fixed tags in comments; <p> included for paragraph comments.
Fixed output of Enum; removed format used for MD generator.

DiegoAstiazaran marked 12 inline comments as done.Jun 25 2019, 12:18 PM

A couple of comments that were related to the function writeDescription() were marked as done since most of that function was deleted. Rendering of the other kinds of CommentInfos will be handled later.

clang-tools-extra/clang-doc/HTMLGenerator.cpp
26

I tried to change it to Twine but the output of OS << genTag() is empty.

34–36

It's a basic paragraph tag used to display text.

DiegoAstiazaran marked 4 inline comments as done.Jun 26 2019, 5:47 PM
DiegoAstiazaran added inline comments.
clang-tools-extra/clang-doc/HTMLGenerator.cpp
107–108

Some of the uses of an intermediate ostream have been deleted but I consider that some are still useful.
One of them is the when rendering the list of Enum Members, after generating a list of <li>'s, we require the intermediate ostream to put it between the ul tags. I consider that's better than:

OS << "<ul>\n";
for (...)
  OS << ...;
OS << "</ul>\n";
DiegoAstiazaran marked an inline comment as done.Jun 26 2019, 5:47 PM

I'm getting confused in all the different patches. I reviewed the HTML generator one like yesterday and was awaiting changes. Doesn't this do the same thing?

I'm getting confused in all the different patches. I reviewed the HTML generator one like yesterday and was awaiting changes. Doesn't this do the same thing?

Diego, why don't you just combine this patch with the structured HTML patch? Would lead to less confusion and churn.

DiegoAstiazaran abandoned this revision.Jun 28 2019, 3:35 PM

I'm getting confused in all the different patches. I reviewed the HTML generator one like yesterday and was awaiting changes. Doesn't this do the same thing?

This revision was supposed to be a basic generator and D63857 added a more structured functionality to it. This revision will be abandoned and everything will be combined in D63857.