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

Roman Lebedev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 6 07:51:10 PST 2018


lebedev.ri added a comment.

Thank you for working on this!

Finally trying to review this...
I must say i'm **really** not fond of the test 'changes'.

But some initial comments added:



================
Comment at: clang-doc/BitcodeReader.cpp:19
+
+void ClangDocBitcodeReader::storeData(llvm::SmallString<4> &Field,
+                                      llvm::StringRef Blob) {
----------------
I think all these `SmallString` can be one `llvm::SmallVectorImpl<char>`?


================
Comment at: clang-doc/BitcodeReader.cpp:454
+    for (const auto &N : (*BlockInfo).getBlockInfo(I)->RecordNames)
+      RecordNames[N.first] = N.second;
+  }
----------------
`RecordNames` is basically not kept between the reuses of `ClangDocBitcodeReader`, but is also not actually properly gc'd.
Also, this uses `BI_FIRST` / `BI_LAST`, which means the versioning is actually absolutely required...
Also, `RecordNames` isn't actually used anywhere later in this code.

So, Is that needed for the next patches? Or why read this at all?




================
Comment at: clang-doc/BitcodeReader.h:36
+
+  bool readBitstream(llvm::SmallString<2048> Bits,
+                     std::unique_ptr<InfoSet> &IS);
----------------
`Bits` is not an output parameter, but just a const input parameter?
I think it would be cleaner to use `StringRef`, to avoid depending on the actual size of the small-size.


================
Comment at: clang-doc/BitcodeReader.h:46
+  bool readBlockInfoBlock(llvm::BitstreamCursor &Stream);
+  template <typename T>
+  bool readBlockToInfo(llvm::BitstreamCursor &Stream, unsigned ID,
----------------
Please separate those tree functions and this template function with one newline.


================
Comment at: clang-doc/BitcodeReader.h:48
+  bool readBlockToInfo(llvm::BitstreamCursor &Stream, unsigned ID,
+                       std::unique_ptr<InfoSet> &IS, T &&I);
+
----------------
 `T &&I` looks super vague.
`T` is `{something}Info`, which is inherited from `Info` base-class.
Maybe something like `InfoT &&I` ?


================
Comment at: clang-doc/Reducer.cpp:19
+  std::unique_ptr<InfoSet> UniqueInfos = llvm::make_unique<InfoSet>();
+  doc::ClangDocBitcodeReader Reader;
+  bool Err = false;
----------------
As far as i can tell, `Reader` isn't required to be kept out here.
It seems it's not used to retain any internal state.
The only obvious reason to keep it, is to avoid de-allocating and then re-allocating the memory each time.

I'm wondering if it would be better to move it into the lambda.
Also, why is it prefixed with the `doc::`? We are in that namespace already.

Then, you will also be able to get rid of `llvm::BitstreamCursor &Stream` params in the `ClangDocBitcodeReader` member functions, like with `ClangDocBitcodeWriter`.


================
Comment at: clang-doc/Representation.h:193
+ private:
+  void resolveReferences(llvm::SmallVector<Reference, 4> &References,
+                         Reference &Caller);
----------------
Similarly, i think those should be `SmallVectorImpl` (i assume those are output params, too?)


================
Comment at: test/clang-doc/mapper-class-in-class.cpp:1
-// RUN: rm -rf %t
-// RUN: mkdir %t
----------------
What's up with all tests being deleted? That is rather disturbing.
I would expect the merge phase to be disable-able, if only just for the testing purposes.


https://reviews.llvm.org/D43341





More information about the cfe-commits mailing list