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

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran abandoned this revision.
DiegoAstiazaran added a comment.

In D63180#1562982 , @jakehehrlich 
wrote:

> 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 
.


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

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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

In D63180#1562982 , @jakehehrlich 
wrote:

> 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.


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

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

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

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?


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

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

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran marked 4 inline comments as done.
DiegoAstiazaran added inline comments.



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

jakehehrlich wrote:
> This is a generally bad pattern.
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 's, we require the intermediate ostream to put it between the ul 
tags. I consider that's better than:

```
OS << "\n";
for (...)
  OS << ...;
OS << "\n";
```


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

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

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran marked 12 inline comments as done.
DiegoAstiazaran added a comment.

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.




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

jakehehrlich wrote:
> This can also be a Twine
I tried to change it to Twine but the output of OS << genTag() is empty.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:33-35
+void writeLine(const Twine , raw_ostream ) {
+  OS << genTag(Text, "p") << "\n";
+}

jakehehrlich wrote:
> I'm not familiar with HTML.  What does this '' do?
It's a basic paragraph tag used to display text.


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

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

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran updated this revision to Diff 206502.
DiegoAstiazaran marked 4 inline comments as done.
DiegoAstiazaran edited the summary of this revision.
DiegoAstiazaran added a comment.

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


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

https://reviews.llvm.org/D63180

Files:
  clang-tools-extra/clang-doc/CMakeLists.txt
  clang-tools-extra/clang-doc/Generators.cpp
  clang-tools-extra/clang-doc/Generators.h
  clang-tools-extra/clang-doc/HTMLGenerator.cpp
  clang-tools-extra/clang-doc/MDGenerator.cpp
  clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
  clang-tools-extra/unittests/clang-doc/CMakeLists.txt
  clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp

Index: clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -0,0 +1,209 @@
+//===-- clang-doc/HTMLGeneratorTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangDocTest.h"
+#include "Generators.h"
+#include "Representation.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace doc {
+
+std::unique_ptr getHTMLGenerator() {
+  auto G = doc::findGeneratorByName("html");
+  if (!G)
+return nullptr;
+  return std::move(G.get());
+}
+
+TEST(HTMLGeneratorTest, emitNamespaceHTML) {
+  NamespaceInfo I;
+  I.Name = "Namespace";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.ChildNamespaces.emplace_back(EmptySID, "ChildNamespace",
+ InfoType::IT_namespace);
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+  I.ChildFunctions.emplace_back();
+  I.ChildFunctions.back().Name = "OneFunction";
+  I.ChildEnums.emplace_back();
+  I.ChildEnums.back().Name = "OneEnum";
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(namespace Namespace
+Namespaces
+ChildNamespace
+Records
+ChildStruct
+Functions
+OneFunction
+ OneFunction()
+Enums
+enum OneEnum
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitRecordHTML) {
+  RecordInfo I;
+  I.Name = "r";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
+
+  I.Members.emplace_back("int", "X", AccessSpecifier::AS_private);
+  I.TagType = TagTypeKind::TTK_Class;
+  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record);
+  I.VirtualParents.emplace_back(EmptySID, "G", InfoType::IT_record);
+
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+  I.ChildFunctions.emplace_back();
+  I.ChildFunctions.back().Name = "OneFunction";
+  I.ChildEnums.emplace_back();
+  I.ChildEnums.back().Name = "OneEnum";
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(class r
+Defined at line 10 of test.cpp
+Inherits from F, G
+Members
+private int X
+Records
+ChildStruct
+Functions
+OneFunction
+ OneFunction()
+Enums
+enum OneEnum
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitFunctionHTML) {
+  FunctionInfo I;
+  I.Name = "f";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
+
+  I.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
+  I.Params.emplace_back("int", "P");
+  I.IsMethod = true;
+  I.Parent = Reference(EmptySID, "Parent", InfoType::IT_record);
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(f
+void f(int P)
+Defined at line 10 of test.cpp
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitEnumHTML) {
+  EnumInfo I;
+  I.Name = "e";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, 

[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 ) {
+  std::string Buffer;
+  llvm::raw_string_ostream Stream(Buffer);
+  bool First = true;
+  for (const auto  : 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  : Refs) {
+if (!First)
+  Stream << ", ";

You can just use ` != Refs.begin()` or ` != ()` 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 , const Twine ) {
+  return "<" + Tag.str() + ">" + Text.str() + "";

This can also be a Twine



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:33-35
+void writeLine(const Twine , raw_ostream ) {
+  OS << genTag(Text, "p") << "\n";
+}

I'm not familiar with HTML.  What does this '' 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

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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:87
+std::string CloseTag = I.SelfClosing ? "/>" : ">";
+writeLine("<" + I.Name + Attrs.str() + CloseTag, OS);
+  } else if (I.Kind == "HTMLEndTagComment") {

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



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:89
+  } else if (I.Kind == "HTMLEndTagComment") {
+writeLine("", OS);
+  } else if (I.Kind == "TextComment") {

Same as above



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:110
+for (const auto  : I.Members)
+  Members << "| " << N << " |\n";
+  writeLine(Members.str(), OS);

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



Comment at: clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp:214-218
+  HTML->Children.back()->Kind = "HTMLStartTagComment";
+  HTML->Children.back()->Name = "li";
+  HTML->Children.emplace_back(llvm::make_unique());
+  HTML->Children.back()->Kind = "TextComment";
+  HTML->Children.back()->Text = " Testing.";

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



Comment at: clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp:287
+Defined at line 10 of test.cpp
+
+ Brief description.

Should the whole comment be wrapped in a  element?


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

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

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran created this revision.
DiegoAstiazaran added reviewers: juliehockett, jakehehrlich, lebedev.ri.
DiegoAstiazaran added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman, mgorny.

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


https://reviews.llvm.org/D63180

Files:
  clang-tools-extra/clang-doc/CMakeLists.txt
  clang-tools-extra/clang-doc/Generators.cpp
  clang-tools-extra/clang-doc/Generators.h
  clang-tools-extra/clang-doc/HTMLGenerator.cpp
  clang-tools-extra/clang-doc/MDGenerator.cpp
  clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
  clang-tools-extra/unittests/clang-doc/CMakeLists.txt
  clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp

Index: clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -0,0 +1,306 @@
+//===-- clang-doc/HTMLGeneratorTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangDocTest.h"
+#include "Generators.h"
+#include "Representation.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace doc {
+
+std::unique_ptr getHTMLGenerator() {
+  auto G = doc::findGeneratorByName("html");
+  if (!G)
+return nullptr;
+  return std::move(G.get());
+}
+
+TEST(HTMLGeneratorTest, emitNamespaceHTML) {
+  NamespaceInfo I;
+  I.Name = "Namespace";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.ChildNamespaces.emplace_back(EmptySID, "ChildNamespace",
+ InfoType::IT_namespace);
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+  I.ChildFunctions.emplace_back();
+  I.ChildFunctions.back().Name = "OneFunction";
+  I.ChildEnums.emplace_back();
+  I.ChildEnums.back().Name = "OneEnum";
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(namespace Namespace
+
+Namespaces
+ChildNamespace
+
+Records
+ChildStruct
+
+Functions
+OneFunction
+ OneFunction()
+
+Enums
+| enum OneEnum |
+--
+
+
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitRecordHTML) {
+  RecordInfo I;
+  I.Name = "r";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
+
+  I.Members.emplace_back("int", "X", AccessSpecifier::AS_private);
+  I.TagType = TagTypeKind::TTK_Class;
+  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record);
+  I.VirtualParents.emplace_back(EmptySID, "G", InfoType::IT_record);
+
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+  I.ChildFunctions.emplace_back();
+  I.ChildFunctions.back().Name = "OneFunction";
+  I.ChildEnums.emplace_back();
+  I.ChildEnums.back().Name = "OneEnum";
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(class r
+Defined at line 10 of test.cpp
+Inherits from F, G
+
+Members
+private int X
+
+Records
+ChildStruct
+
+Functions
+OneFunction
+ OneFunction()
+
+Enums
+| enum OneEnum |
+--
+
+
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitFunctionHTML) {
+  FunctionInfo I;
+  I.Name = "f";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
+
+  I.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
+  I.Params.emplace_back("int", "P");
+  I.IsMethod = true;
+  I.Parent = Reference(EmptySID, "Parent", InfoType::IT_record);
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(f
+void f(int P)
+Defined at line 10 of test.cpp
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitEnumHTML) {
+  EnumInfo I;
+  I.Name = "e";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
+
+