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