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