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