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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
Comment at: clang-doc/BitcodeReader.cpp:27
+  assert(Record[0] == 20);
+  for (int I = 0, E = Record[0]; I < E; ++I)
+    Field[I] = Record[I + 1];
----------------
lebedev.ri wrote:
> Ok, i don't understand what is going on here.
> Where is this `E` defined?
> This looks like `[0]` is the number of elements to read (always 20, sha1 blob?),
> and it copies Record[1]..Record[20] to Field[0]..Field[19] ?
> I think this can/should be rewritten clearer..
`Record[0]` holds the size of the array -- in this case, 20 -- in the bitstream representation of the array (see the array section [[ https://llvm.org/docs/BitCodeFormat.html#define-abbrev-encoding | here ]]). So, yes, it copies Record[1]...[20] to Field[0]...[19]. Not sure how that can be rewritten more clearly, but I'll add a clarifying comment!


================
Comment at: clang-doc/BitcodeWriter.h:129
     // Check that the static size is large-enough.
     assert(Record.capacity() > BitCodeConstants::RecordSize);
   }
----------------
lebedev.ri wrote:
> Isn't that the opposite of what was that assert supposed to do?
> It would have been better to just `// disable` it, and add a `FIXME` note.
I'm not sure I'm understanding you -- my understanding was that it existed to check that the record size was large enough.


================
Comment at: clang-doc/Reducer.cpp:18
+std::unique_ptr<InfoSet> mergeInfos(tooling::ToolResults *Results) {
+  std::unique_ptr<InfoSet> UniqueInfos = llvm::make_unique<InfoSet>();
+  bool Err = false;
----------------
lebedev.ri wrote:
> I can see that you may `return nullptr;` in case of error here, thus it's `std::unique_ptr<>`
> `InfoSet` internally is just a few `std::vector<>`s, so it should `std::move()` efficiently.
> I'm wondering if `llvm::Optional<InfoSet>` would work too?
Doesn't `llvm::Optional` imply that it *could* validly not exist though? This is trying to express here that that isn't a valid state.


================
Comment at: clang-doc/Representation.h:135
   Info() = default;
-  Info(Info &&Other) : Description(std::move(Other.Description)) {}
-  virtual ~Info() = default;
+  Info(Info &&Other)
+      : USR(Other.USR), Name(Other.Name), Namespace(std::move(Other.Namespace)),
----------------
lebedev.ri wrote:
> Why is the move constructor explicitly defined?
> Unless i'm missing something, the default one would do exactly the same.
> https://godbolt.org/g/XqsRuX
It's there (as is the CommentInfo one) because otherwise it was throwing errors. It appears that a copy constructor is called somewhere if this isn't specified.


https://reviews.llvm.org/D43341