[PATCH] D63180: [clang-doc] Adds HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

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.



================
Comment at: clang-tools-extra/clang-doc/Generators.cpp:60-71
+std::string genReferenceList(const llvm::SmallVectorImpl<Reference> &Refs) {
+  std::string Buffer;
+  llvm::raw_string_ostream Stream(Buffer);
+  bool First = true;
+  for (const auto &R : Refs) {
+    if (!First)
+      Stream << ", ";
----------------
This seems to be displaying a list in a particular way. Mind adding a comment about where this is used?


================
Comment at: clang-tools-extra/clang-doc/Generators.cpp:65
+  for (const auto &R : Refs) {
+    if (!First)
+      Stream << ", ";
----------------
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.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:25
+
+std::string genTag(const Twine &Text, const Twine &Tag) {
+  return "<" + Tag.str() + ">" + Text.str() + "</" + Tag.str() + ">";
----------------
This can also be a Twine


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:33-35
+void writeLine(const Twine &Text, raw_ostream &OS) {
+  OS << genTag(Text, "p") << "\n";
+}
----------------
I'm not familiar with HTML.  What does this '<p>' do?


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:66
+    OS << genEmphasis(I.ParamName) << I.Text << Direction << "\n\n";
+  } else if (I.Kind == "TParamCommandComment") {
+    std::string Direction = I.Explicit ? (" " + I.Direction).str() : "";
----------------
Instead of repeating code add an "||" case to the above.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:67-68
+  } else if (I.Kind == "TParamCommandComment") {
+    std::string Direction = I.Explicit ? (" " + I.Direction).str() : "";
+    OS << genEmphasis(I.ParamName) << I.Text << Direction << "\n\n";
+  } else if (I.Kind == "VerbatimBlockComment") {
----------------
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.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:72-77
+  } else if (I.Kind == "VerbatimBlockLineComment") {
+    OS << I.Text;
+    writeNewLine(OS);
+  } else if (I.Kind == "VerbatimLineComment") {
+    OS << I.Text;
+    writeNewLine(OS);
----------------
Dedup these.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:81-84
+    std::string Buffer;
+    llvm::raw_string_ostream Attrs(Buffer);
+    for (unsigned Idx = 0; Idx < I.AttrKeys.size(); ++Idx)
+      Attrs << " \"" << I.AttrKeys[Idx] << "=" << I.AttrValues[Idx] << "\"";
----------------
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.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:86
+
+    std::string CloseTag = I.SelfClosing ? "/>" : ">";
+    writeLine("<" + I.Name + Attrs.str() + CloseTag, OS);
----------------
This can be a StringRef


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:106-107
+
+  std::string Buffer;
+  llvm::raw_string_ostream Members(Buffer);
+  if (!I.Members.empty())
----------------
This is a generally bad pattern.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63180/new/

https://reviews.llvm.org/D63180