[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-doc/Index.h:28
+// Abstract class representing an index of mapped info objects.
+class Index {
+public:
----------------
What is this interface for? It looks like none of the functions are ever called through the interface.

If the intent is to abstract away a MR framework, I don't think this achieves that - multimachine MR frameworks have their "index" distributed and probably won't have a "reduce" function you can call with these semantics.

Consider just moving the in-memory reduction to the main file (which isn't portable across MR abstractions anyway) and avoiding abstractions there:
```
StringMap<std::vector<std::unique_ptr<Info>>>>  MapOutput;
// populate MapOutput as you currently do in inMemoryReduceResults
for (const auto &Group : MapOutput)
  Write(Group.first, reduceInfos(std::move(Group.second)));
```


================
Comment at: clang-doc/Reducer.cpp:18
+
+#define REDUCE(INFO, TYPE)                                                     \
+  {                                                                            \
----------------
TYPE is unused?


================
Comment at: clang-doc/Representation.cpp:1
+///===-- Representation.cpp - ClangDoc Representation -----------*- C++ -*-===//
+//
----------------
this is important/subtle logic, and there are no comments (particularly about motivating how specific fields are merged)!


================
Comment at: clang-doc/Representation.cpp:15
+
+template <typename T> void assign(T &L, T &R) {
+  if (L != R)
----------------
why this, rather than actual assignment? (comments!)


================
Comment at: clang-doc/Representation.cpp:20
+
+template <typename T> void move(T &L, T &&R) {
+  if (L != R)
----------------
why would you ever assign() rather than move()


================
Comment at: clang-doc/Representation.cpp:26
+template <>
+void move(llvm::SmallVectorImpl<Reference> &L,
+          llvm::SmallVectorImpl<Reference> &&R) {
----------------
why this pattern of "assign if empty" for these types?


================
Comment at: clang-doc/Representation.cpp:39
+template <typename T>
+void extend(llvm::SmallVectorImpl<T> &L, llvm::SmallVectorImpl<T> &&R) {
+  std::move(R.begin(), R.end(), std::back_inserter(L));
----------------
what if there are duplicatel?


================
Comment at: clang-doc/Representation.cpp:53
+  move(Namespace, std::move(Other.Namespace));
+  extend(Description, std::move(Other.Description));
+  return true;
----------------
is plain concatenation of comments what you want?


================
Comment at: clang-doc/Representation.h:75
 
+  bool operator==(const Reference &Other) const {
+    return USR == Other.USR && UnresolvedName == Other.UnresolvedName &&
----------------
consider `std::tie(A, B) == std::tie(Other.A, Other.B)`


================
Comment at: clang-doc/Representation.h:79
+  }
+  bool operator!=(const Reference &Other) const {
+    return USR != Other.USR || UnresolvedName != Other.UnresolvedName ||
----------------
do you really need `!=`?
if so, prefer to define it as `!(*this == Other)` to avoid redundancy.


================
Comment at: clang-doc/Representation.h:159
+  Info(InfoType IT) : IT(IT) {}
+  Info(Info &Other) = delete;
+  Info(Info &&Other) = default;
----------------
should this be a copy constructor? (reference should be const)


================
Comment at: clang-doc/Representation.h:162
+
+  bool merge(Info &&I);
 
----------------
So having this "overriding" that's not virtual seems like asking for trouble: if you accidentally deref a unique_ptr<Info> and compare it, you'll get the base behavior which should probably never be called directly.

Consider naming this `mergeBase` and making it protected.


https://reviews.llvm.org/D43341