[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

124–127 ↗

(On Diff #203404)

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

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

51–59 ↗

(On Diff #203404)

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

[jakehehrlich]

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

124 ↗

(On Diff #203404)

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

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

60–65 ↗

(On Diff #203404)

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

139–140 ↗

(On Diff #203404)

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

124 ↗

(On Diff #203404)

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 ↗

(On Diff #204146)

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 ↗

(On Diff #204146)

Same comment here.

[DiegoAstiazaran]

Add comments.

clang-tools-extra/trunk/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>{});
+  }
+
+  // This operator is used to sort a vector of CommentInfos.
+  // No specific order (attributes more important than others) is required. Any
+  // sort is enough, the order is only needed to call std::unique after sorting
+  // the vector.
+  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.
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);
   }
 
+  // This operator is used to sort a vector of Locations.
+  // No specific order (attributes more important than others) is required. Any
+  // sort is enough, the order is only needed to call std::unique after sorting
+  // the vector.
+  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/trunk/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/trunk/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;