[clang-doc] De-duplicate comments and locations

[DiegoAstiazaran]

Summary

De-duplicate comments and declaration locations in reduce function.
When two files include the same header file, this file's content is mapped twice causing comments and locations to be duplicated after the reduce stage.

[lebedev.ri]

Test?

[juliehockett]

Please add a test case to unittests/clang-doc/MergeTest.cpp.

clang-tools-extra/clang-doc/Representation.cpp

125–128

if (std::find(Description.begin(), Description.end(), Comment) == Description.end())
  Description.emplace_back(std::move(Comment));

clang-tools-extra/clang-doc/Representation.h

86–94

Use std::tie here (see Reference::operator== below as an example).

[jakehehrlich]

clang-tools-extra/clang-doc/Representation.cpp

125

Instead of deduping like this can we just make Description a set?

clang-tools-extra/clang-doc/Representation.h

95–100

You can do this using llvm::make_pointer_range/llvm::pointer_iterator and std::equal

[jakehehrlich]

Actually if we can make Description a hash set that would be best. I think using std::set would require an awkward < operator to be defined. There should be standard ways of getting hashes out of all the string and vector types and you can use hash_combine to combine all of these into a single hash.

[juliehockett]

In D62970#1533229, @jakehehrlich wrote:

Actually if we can make Description a hash set that would be best. I think using std::set would require an awkward < operator to be defined. There should be standard ways of getting hashes out of all the string and vector types and you can use hash_combine to combine all of these into a single hash.

This SGTM -- this would actually make it a bit more deterministic in the ordering, since we'd no longer depend on the order of file load, even.

clang-tools-extra/clang-doc/Representation.cpp

138–142

While you're here, could you actually do the same for Location? That is, make it a hash set on SymbolInfo and dedup here?

[DiegoAstiazaran]

Change how the vectors are de-duplicated.
Add de-duplication of declaration locations.
Add tests.

[DiegoAstiazaran]

clang-tools-extra/clang-doc/Representation.cpp

125

The YAML generator uses an llvm method that doesn't support sets, so changing Description to a set would break that.
Talking with Julie we decided to de-duplicate with sort and unique.

[jakehehrlich]

LGTM

clang-tools-extra/clang-doc/Representation.h

66

We should be explicit about what we intend to happen here. Do we just want a total ordering of any kind? Do we want some things to be "more important" than others? For instance because the ordering is lexicographical here Kind is more important than Text which is more important than Name etc...

If the intent is just to have any total order state so at the top and why we need a total ordering (for instance to have an std::set/std::map of these things or we need to sort them, etc...)

186

Same comment here.

[DiegoAstiazaran]

Add comments.

clang-tools-extra/clang-doc/Representation.h

enum class InfoType {
   IT_enum
 };
 
 // A representation of a parsed comment.
 struct CommentInfo {
   CommentInfo() = default;
   CommentInfo(CommentInfo &Other) = delete;
   CommentInfo(CommentInfo &&Other) = default;
+  CommentInfo &operator=(CommentInfo &&Other) = default;
+
+  bool operator==(const CommentInfo &Other) const {
+    auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,
+                            SelfClosing, Explicit, AttrKeys, AttrValues, Args);
+    auto SecondCI =
+        std::tie(Other.Kind, Other.Text, Other.Name, Other.Direction,
+                 Other.ParamName, Other.CloseName, Other.SelfClosing,
+                 Other.Explicit, Other.AttrKeys, Other.AttrValues, Other.Args);
+
+    if (FirstCI != SecondCI || Children.size() != Other.Children.size())
+      return false;
+
+    return std::equal(Children.begin(), Children.end(), Other.Children.begin(),
+                      llvm::deref<llvm::equal>{});
+  }
+
+  bool operator<(const CommentInfo &Other) const {
+    auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,
+                            SelfClosing, Explicit, AttrKeys, AttrValues, Args);
+    auto SecondCI =
+        std::tie(Other.Kind, Other.Text, Other.Name, Other.Direction,
+                 Other.ParamName, Other.CloseName, Other.SelfClosing,
+                 Other.Explicit, Other.AttrKeys, Other.AttrValues, Other.Args);
+
+    if (FirstCI < SecondCI ||
+        (FirstCI == SecondCI && Children.size() < Other.Children.size()))
+      return true;
+
+    if (FirstCI > SecondCI || Children.size() > Other.Children.size())
+      return false;
+
+    return std::equal(Children.begin(), Children.end(), Other.Children.begin(),
+                      llvm::deref<llvm::less>{});
+  }
 
   SmallString<16>
       Kind; // Kind of comment (FullComment, ParagraphComment, TextComment,
             // InlineCommandComment, HTMLStartTagComment, HTMLEndTagComment,
             // BlockCommandComment, ParamCommandComment,
             // TParamCommandComment, VerbatimBlockComment,
             // VerbatimBlockLineComment, VerbatimLineComment).
   SmallString<64> Text;      // Text of the comment.
   SmallString<16> Name;      // Name of the comment (for Verbatim and HTML).
   SmallString<8> Direction;  // Parameter direction (for (T)ParamCommand).
   SmallString<16> ParamName; // Parameter name (for (T)ParamCommand).
   SmallString<16> CloseName; // Closing tag name (for VerbatimBlock).
   bool SelfClosing = false;  // Indicates if tag is self-closing (for HTML).
   bool Explicit = false; // Indicates if the direction of a param is explicit
                          // (for (T)ParamCommand).
   llvm::SmallVector<SmallString<16>, 4>
       AttrKeys; // List of attribute keys (for HTML).
   llvm::SmallVector<SmallString<16>, 4>
       AttrValues; // List of attribute values for each key (for HTML).
   llvm::SmallVector<SmallString<16>, 4>
       Args; // List of arguments to commands (for InlineCommand).
   std::vector<std::unique_ptr<CommentInfo>>
       Children; // List of child comments for this CommentInfo.
 };
 
struct Location {
   Location(int LineNumber, SmallString<16> Filename)
       : LineNumber(LineNumber), Filename(std::move(Filename)) {}
 
   bool operator==(const Location &Other) const {
     return std::tie(LineNumber, Filename) ==
            std::tie(Other.LineNumber, Other.Filename);
   }
 
+  bool operator<(const Location &Other) const {
+    return std::tie(LineNumber, Filename) <
+           std::tie(Other.LineNumber, Other.Filename);
+  }
+
   int LineNumber;           // Line number of this Location.
   SmallString<32> Filename; // File for this Location.
 };
 
 /// A base struct for Infos.
 struct Info {
   Info() = default;
   Info(InfoType IT) : IT(IT) {}

clang-tools-extra/clang-doc/Representation.cpp

if (USR == EmptySID)
     USR = Other.USR;
   if (Name == "")
     Name = Other.Name;
   if (Namespace.empty())
     Namespace = std::move(Other.Namespace);
   // Unconditionally extend the description, since each decl may have a comment.
   std::move(Other.Description.begin(), Other.Description.end(),
             std::back_inserter(Description));
+  std::sort(Description.begin(), Description.end());
+  auto Last = std::unique(Description.begin(), Description.end());
+  Description.erase(Last, Description.end());
 }
 
 bool Info::mergeable(const Info &Other) {
   return IT == Other.IT && USR == Other.USR;
 }
 
 void SymbolInfo::merge(SymbolInfo &&Other) {
   assert(mergeable(Other));
   if (!DefLoc)
     DefLoc = std::move(Other.DefLoc);
   // Unconditionally extend the list of locations, since we want all of them.
   std::move(Other.Loc.begin(), Other.Loc.end(), std::back_inserter(Loc));
+  std::sort(Loc.begin(), Loc.end());
+  auto Last = std::unique(Loc.begin(), Loc.end());
+  Loc.erase(Last, Loc.end());
   mergeBase(std::move(Other));
 }
 
 void NamespaceInfo::merge(NamespaceInfo &&Other) {
   assert(mergeable(Other));
   // Reduce children if necessary.
   reduceChildren(ChildNamespaces, std::move(Other.ChildNamespaces));
   reduceChildren(ChildRecords, std::move(Other.ChildRecords));

clang-tools-extra/unittests/clang-doc/MergeTest.cpp

TEST(MergeTest, mergeFunctionInfos) {
   One.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   One.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   One.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
 
   One.IsMethod = true;
   One.Parent = Reference(EmptySID, "Parent", InfoType::IT_namespace);
 
+  One.Description.emplace_back();
+  auto OneFullComment = &One.Description.back();
+  OneFullComment->Kind = "FullComment";
+  auto OneParagraphComment = llvm::make_unique<CommentInfo>();
+  OneParagraphComment->Kind = "ParagraphComment";
+  auto OneTextComment = llvm::make_unique<CommentInfo>();
+  OneTextComment->Kind = "TextComment";
+  OneTextComment->Text = "This is a text comment.";
+  OneParagraphComment->Children.push_back(std::move(OneTextComment));
+  OneFullComment->Children.push_back(std::move(OneParagraphComment));
+
   FunctionInfo Two;
   Two.Name = "f";
   Two.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
-  Two.Loc.emplace_back(20, llvm::SmallString<16>{"test.cpp"});
+  Two.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
 
   Two.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   Two.Params.emplace_back("int", "P");
 
+  Two.Description.emplace_back();
+  auto TwoFullComment = &Two.Description.back();
+  TwoFullComment->Kind = "FullComment";
+  auto TwoParagraphComment = llvm::make_unique<CommentInfo>();
+  TwoParagraphComment->Kind = "ParagraphComment";
+  auto TwoTextComment = llvm::make_unique<CommentInfo>();
+  TwoTextComment->Kind = "TextComment";
+  TwoTextComment->Text = "This is a text comment.";
+  TwoParagraphComment->Children.push_back(std::move(TwoTextComment));
+  TwoFullComment->Children.push_back(std::move(TwoParagraphComment));
+
   std::vector<std::unique_ptr<Info>> Infos;
   Infos.emplace_back(llvm::make_unique<FunctionInfo>(std::move(One)));
   Infos.emplace_back(llvm::make_unique<FunctionInfo>(std::move(Two)));
 
   auto Expected = llvm::make_unique<FunctionInfo>();
   Expected->Name = "f";
   Expected->Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   Expected->DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   Expected->Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
-  Expected->Loc.emplace_back(20, llvm::SmallString<16>{"test.cpp"});
 
   Expected->ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   Expected->Params.emplace_back("int", "P");
   Expected->IsMethod = true;
   Expected->Parent = Reference(EmptySID, "Parent", InfoType::IT_namespace);
 
+  Expected->Description.emplace_back();
+  auto ExpectedFullComment = &Expected->Description.back();
+  ExpectedFullComment->Kind = "FullComment";
+  auto ExpectedParagraphComment = llvm::make_unique<CommentInfo>();
+  ExpectedParagraphComment->Kind = "ParagraphComment";
+  auto ExpectedTextComment = llvm::make_unique<CommentInfo>();
+  ExpectedTextComment->Kind = "TextComment";
+  ExpectedTextComment->Text = "This is a text comment.";
+  ExpectedParagraphComment->Children.push_back(std::move(ExpectedTextComment));
+  ExpectedFullComment->Children.push_back(std::move(ExpectedParagraphComment));
+
   auto Actual = mergeInfos(Infos);
   assert(Actual);
   CheckFunctionInfo(InfoAsFunction(Expected.get()),
                     InfoAsFunction(Actual.get().get()));
 }
 
 TEST(MergeTest, mergeEnumInfos) {
   EnumInfo One;