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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits