Page MenuHomePhabricator

[clang-doc] Add a structured HTML generator
ClosedPublic

Authored by DiegoAstiazaran on Jun 26 2019, 5:31 PM.

Details

Summary

Implements an HTML generator.
Nodes are used to represent each part of the HTML file. There are TagNodes that represent every HTML tag (p, h1, div, ...) and they have children nodes, which can be TagNodes or TextNodes (these nodes only have text).
Proper indentation is rendered within the files generated by tool.
No styling (CSS) is included.

Diff Detail

Repository
rL LLVM

Event Timeline

juliehockett added inline comments.Jun 26 2019, 6:00 PM
clang-tools-extra/clang-doc/HTMLGenerator.cpp
21 ↗(On Diff #206774)

Put the class definitions in an anonymous namespace (https://llvm.org/docs/CodingStandards.html#anonymous-namespaces)

24 ↗(On Diff #206774)

Use TagType or some such to avoid confusion with the containing class

25–33 ↗(On Diff #206774)

Enum values should be capitalized, and probably prefixed with HTML_ or TAG_ or some such useful prefix (e.g. TAG_META)

42 ↗(On Diff #206774)

For this and other functions, separate the implementation from the definition.

121 ↗(On Diff #206774)
TagNode(HTMLTag Tag) : Tag(Tag), InlineChildren(Tag.HasInlineChildren), SelfClosing(Tag.SelfClosing) {}
412 ↗(On Diff #206774)

use emplace_back here for instantiation

clang-tools-extra/clang-doc/MDGenerator.cpp
31 ↗(On Diff #206774)

Can you re-static these?

DiegoAstiazaran marked 7 inline comments as done.

Fix TagType enum name and members.
Add anonymous namespace.
Separate the implementation from the definition for some functions.
Use emplace_back instead of push_back for instantiation of vector members.

Overall looks better. One of my comments causes a sweeping change to occur so I'll respond back after thats done.

clang-tools-extra/clang-doc/HTMLGenerator.cpp
91 ↗(On Diff #206920)

Does this ever change? If not this should be a global constexpr constant char* inside of an anon-namespace.

e.g.

namespace {

constexpr const char* kDoctypeDecl = "...";

}

170 ↗(On Diff #206920)

You can exit here if its self closing right?

187 ↗(On Diff #206920)

You should return TagNodes, not assign to them by ref. Preferably via retruning a unique pointer. It doesn't make sense to have the consumer do the constructor I would imagine. For instance what tag is the right tag type? The consumer shouldn't decide, the producer should.

190 ↗(On Diff #206920)

Same comment here these function should look like (at a very high an imprecise level, obviously some functions will be more complicated.)

std::unique_ptr<TagNode> genFoo(info) {
  auto out = llvm::make_unique<...>(...);
  for (const auto &B : info.bars)
    out->Children.emplace_back(genBar(B));
  return out;
}

This goes for all of these functions

DiegoAstiazaran marked 6 inline comments as done.

Make DoctypeDecl a constexpr.
Early exit in TagNode::Render.
Functions of HTMLGenerator now return HTMLNodes instead of adding children to a Node passed by reference.

clang-tools-extra/clang-doc/HTMLGenerator.cpp
91 ↗(On Diff #206920)

In HTML5 it's always <!DOCTYPE html> so I've already changed it in the latest patch.

170 ↗(On Diff #206920)

Yes, fixed in the latest patch update.

Can you upload git diff -U100000 <commit before genHTML> functions so that I can see the code without it being fragmented? It's hard to parse some of the more fragmented stuff here

clang-tools-extra/clang-doc/HTMLGenerator.cpp
191–194 ↗(On Diff #207148)

Never use 'static' for functions in C++. For functions anon-namespaces and 'static' mean the same thing but C++ generates a lot of stuff behind the scenes and to make those 'static' you need to use an non-namespace.

In general the rule is to put as much as you can in anon-namespaces and always prefer that for functions/globals/constants over 'static'. This is important for constexpr variables specifically because depending on the C++ version they don't play nice with the static keyword.

DiegoAstiazaran retitled this revision from [clang-doc] Structured HTML generator to [clang-doc] Add a structured HTML generator.
DiegoAstiazaran edited the summary of this revision. (Show Details)

This patch depended on D63180, that revision has been abandoned and combined with this one.

jakehehrlich added inline comments.Jun 28 2019, 3:42 PM
clang-tools-extra/clang-doc/HTMLGenerator.cpp
228 ↗(On Diff #207148)

Refering to this as Out.back() is confusing, can you give it a variable name? It took me longer than I care to admit to figure out that you weren't appending to Out in the loop. A pretty common pattern I use is to just assign back() to a reference that I'm going to use a lot.

Out.emplace_back(...);
auto& DivBody = Out.back();

That way you get the inplace construction and you can refer to it wherever you'd like by name.

DiegoAstiazaran marked 2 inline comments as done.

Add a variable to use as a reference to the last item in a vector instead of doing vector.back().

This looks good to me!

clang-tools-extra/clang-doc/Generators.cpp
75–77 ↗(On Diff #207174)

I don't fully understand these lines. Can you explain how they work and what they accomplish?

clang-tools-extra/clang-doc/HTMLGenerator.cpp
75 ↗(On Diff #207174)

I think you can just write Children.emplace_back(Text.str(), !InlineChildren)

310–312 ↗(On Diff #207174)
for (...)  
  if (std::unique)ptr<...> Node = genHTML(Child))
    CommentBlock->...

would be a bitmore idiomatic LLVM code. I think you used this pattern above as well so you should fix it there as well.

325 ↗(On Diff #207174)

same thing here I think.

358 ↗(On Diff #207174)

ditto

422 ↗(On Diff #207174)

ditto

425 ↗(On Diff #207174)

ditto

428 ↗(On Diff #207174)

ditto

432–434 ↗(On Diff #207174)

You use this a lot here and once or twice above. You could write a template function that takes an std::vector<B>&& and moves its contents into some other std::vector<A>& where B : A

You can use SFINAE to ensure that B is subtype of A.

472–475 ↗(On Diff #207174)

These are all cases of what I was talking about before.

505 ↗(On Diff #207174)

ditto.

507 ↗(On Diff #207174)

Use emplace_back when moving to avoid extra constructor calls.

As in I'm not quite ready for this to land but It looks preety solid. I think if you respond to those comments I'll take one last look and then give an actual accept

DiegoAstiazaran marked 5 inline comments as done.

Add helper function to move a vector's elements to another vector.

clang-tools-extra/clang-doc/HTMLGenerator.cpp
75 ↗(On Diff #207174)

I think it is necessary to explicitly pass a unique_ptr, not only the args.

juliehockett accepted this revision.Jul 9 2019, 9:01 AM

LGTM, unless Jake has further comments.

clang-tools-extra/clang-doc/HTMLGenerator.cpp
503 ↗(On Diff #207174)

Remove this

This revision is now accepted and ready to land.Jul 9 2019, 9:01 AM
DiegoAstiazaran marked an inline comment as done.

Remove empty line.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 12:04 PM