lebedev.ri added a comment. Could some other people please review this differential, too? I'm sure i have missed things.
--- Some more nitpicking. For this differential as standalone, i'we mostly run out of things to nitpick. Some things can probably be done better (the blockid/recordid stuff could probably be nicer if tablegen-ed, but that is for later). I'll try to look at the next differential, and at them combined. ================ Comment at: clang-doc/BitcodeWriter.cpp:120 + BlockIdNameMap[Init.first] = Init.second; + assert((BlockIdNameMap[Init.first].size() + 1) <= + BitCodeConstants::RecordSize); ---------------- We don't actually push these strings to the `Record` (but instead output them directly), so this assertion is not really meaningful, i think? ================ Comment at: clang-doc/BitcodeWriter.h:21 +#include "clang/AST/AST.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/Bitcode/BitstreamWriter.h" ---------------- +DenseMap ================ Comment at: clang-doc/BitcodeWriter.h:21 +#include "clang/AST/AST.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/Bitcode/BitstreamWriter.h" ---------------- lebedev.ri wrote: > +DenseMap +StringRef ================ Comment at: clang-doc/BitcodeWriter.h:197 + : Stream(Stream_) { + Stream.EnterSubblock(ID, BitCodeConstants::SubblockIDSize); + } ---------------- Humm, you could avoid this constant, and conserve a few bits, if you move the init-list out of `emitBlockInfoBlock()` to somewhere e.g. after the `enum RecordId`, and then since the `BlockId ID` is already passed, you could compute it on-the-fly the same way the `BitCodeConstants::SubblockIDSize` is asserted in `emitBlockInfo*()`. Not sure if it's worth doing though. Maybe just add it as a `NOTE` here. ================ Comment at: clang-doc/BitcodeWriter.h:249 +/// +/// \param WriteBlockInfo +/// For serializing a single info (as in the mapper ---------------- Stale comment ================ Comment at: clang-doc/Representation.h:60 + InfoType RefType = InfoType::IT_default; + Info *Ref; +}; ---------------- `Info *Ref;` isn't used anywhere ================ Comment at: clang-doc/Representation.h:117 + bool IsDefinition = false; + Location DefLoc; + llvm::SmallVector<Location, 2> Loc; ---------------- `llvm::Optional<Location> DefLoc;` ? https://reviews.llvm.org/D41102 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits