juliehockett added inline comments.
================
Comment at: clang-doc/BitcodeWriter.cpp:196
+/// \brief Emits a record name to the BLOCKINFO block.
+void ClangDocBitcodeWriter::emitRecordID(RecordId ID) {
+ assert(RecordIdNameMap[ID] && "Unknown Abbreviation");
----------------
lebedev.ri wrote:
> juliehockett wrote:
> > lebedev.ri wrote:
> > > Hmm, so i've been staring at this and
> > > http://llvm.org/doxygen/classllvm_1_1BitstreamWriter.html and i must say
> > > i'm not fond of this indirection.
> > >
> > > What i don't understand is, in previous function, we don't store
> > > `BlockId`, why do we want to store `RecordId`?
> > > Aren't they both unstable, and are implementation detail?
> > > Do we want to store it (`RecordId`)? If yes, please explain it as a new
> > > comment in code.
> > >
> > > If no, i guess this would work too?
> > > ```
> > > assert(RecordIdNameMap[ID] && "Unknown Abbreviation");
> > > Record.clear();
> > > Stream.EmitRecord(llvm::bitc::BLOCKINFO_CODE_SETRECORDNAME,
> > > RecordIdNameMap[ID].Name);
> > > ```
> > > And after that you can lower the default size of `SmallVector<> Record`
> > > down to, hm, `4`?
> > I'm not entirely certain what you mean -- in `emitBlockId()`, we are
> > storing both the block id and the block name in separate records
> > (`BLOCKINFO_CODE_SETBID`, `BLOCKINFO_CODE_BLOCKNAME`, respectively). In
> > `emitRecordId()`, we're doing something slightly different, in that we emit
> > one record with both the record id and the record name (in record
> > `BLOCKINFO_CODE_SETRECORDNAME`).
> >
> > Replacing the copy loop here has the same issue as above, namely that there
> > isn't an easy way to convert between a `StringRef` and an array of
> > `unsigned char`.
> Tried locally, and yes, we do need to output record id.
>
> What we could **actually** do, is simply inline that `EmitRecord()`, first
> emitting the RID, and then the name.
> ```
> template <typename Container>
> void EmitRecord(unsigned Code, int ID, const Container &Vals) {
> // If we don't have an abbrev to use, emit this in its fully unabbreviated
> // form.
> auto Count = static_cast<uint32_t>(makeArrayRef(Vals).size());
> EmitCode(bitc::UNABBREV_RECORD);
> EmitVBR(Code, 6);
> EmitVBR(Count + 1, 6); // Including ID
> EmitVBR64(ID, 6); // 'Prefix' with ID
> for (unsigned i = 0, e = Count; i != e; ++i)
> EmitVBR64(Vals[i], 6);
> }
> ```
>
> But that will result in rather ugly code.
> So given that the record names are quite short, and all the other strings we
> output directly, maybe leave it as it is for now, until it shows in profiles?
>
If that makes sense to you, sounds good to me!
================
Comment at: clang-doc/BitcodeWriter.cpp:139
+ {COMMENT_NAME, {"Name", &StringAbbrev}},
+ {COMMENT_POSITION, {"Position", &IntAbbrev}},
+ {COMMENT_DIRECTION, {"Direction", &StringAbbrev}},
----------------
lebedev.ri wrote:
> This change is not covered by tests.
> (I've actually found out that the hard way, by trying to find why it didn't
> trigger any asssertions, oh well)
So after a some digging, this particular field can't be tested right now as the
mapper doesn't look at any `TemplateDecl`s (something that definitely needs to
be implemented, but in a follow-on patch). I've removed it for now, until it
can be properly used/tested.
================
Comment at: clang-doc/BitcodeWriter.h:37
+ static constexpr unsigned SubblockIDSize = 4U;
+ static constexpr unsigned BoolSize = 1U;
+ static constexpr unsigned IntSize = 16U;
----------------
lebedev.ri wrote:
> juliehockett wrote:
> > lebedev.ri wrote:
> > > Hmm, you build with asserts enabled, right?
> > > I tried testing this, and three tests fail with
> > > ```
> > > clang-doc: /build/llvm/include/llvm/Bitcode/BitstreamWriter.h:122: void
> > > llvm::BitstreamWriter::Emit(uint32_t, unsigned int): Assertion `(Val &
> > > ~(~0U >> (32-NumBits))) == 0 && "High bits set!"' failed.
> > > ```
> > > ```
> > > Failing Tests (3):
> > > Clang Tools :: clang-doc/mapper-class-in-function.cpp
> > > Clang Tools :: clang-doc/mapper-function.cpp
> > > Clang Tools :: clang-doc/mapper-method.cpp
> > >
> > > Expected Passes : 6
> > > Unexpected Failures: 3
> > > ```
> > > At least one failure is because of `BoolSize`, so i'd suspect the
> > > assertion itself is wrong...
> > I do, and I've definitely seen that one triggered before but it's been
> > because something was off in how the data was being outputted as I was
> > shifting things around. That said, I'm not seeing it in my local build with
> > this diff though -- I'll update it again just to make sure they're in sync.
> I did not retry with updated tree/patch, but i'm quite sure i did hit those
> asserts.
> My current build line:
> ```
> -DCMAKE_BUILD_TYPE:STRING=RelWithDebInfo
> -DLLVM_BINUTILS_INCDIR:PATH=/usr/include
> -DLLVM_BUILD_TESTS:BOOL=ON
> -DLLVM_ENABLE_ASSERTIONS:BOOL=ON
> -DLLVM_ENABLE_LLD:BOOL=ON
> -DLLVM_ENABLE_PROJECTS:STRING=clang;libcxx;libcxxabi;compiler-rt;lld
> -DLLVM_ENABLE_SPHINX:BOOL=ON
> -DLLVM_ENABLE_WERROR:BOOL=ON
> -DLLVM_PARALLEL_LINK_JOBS:STRING=1
> -DLLVM_TARGETS_TO_BUILD:STRING=X86
> -DLLVM_USE_SANITIZER:STRING=Address
> ```
> Additional env variables:
> ```
> export MALLOC_CHECK_=3
> export MALLOC_PERTURB_=$(($RANDOM % 255 + 1))
> export ASAN_OPTIONS=abort_on_error=1
> export UBSAN_OPTIONS=print_stacktrace=1
> ```
Figured it out -- the `Reference` struct didn't have default for the enum, and
so if it wasn't initialized it was undefined. Should be fixed now.
================
Comment at: test/clang-doc/mapper-enum.cpp:17
+ // CHECK: <IsDefinition abbrevid=7 op0=1/>
+ // CHECK: <TypeBlock NumWords=4 BlockCodeSize=4>
+ // CHECK: <Type abbrevid=4 op0=3 op1=1/> blob data = 'X'
----------------
lebedev.ri wrote:
> Can `TypeBlock` be on the same depth as `VersionBlock`?
> Via `using`/`typename`? If yes, please add such a test.
Not currently -- I'm planning to add that functionality in the future, but
right now it ignores typedef or using decls.
https://reviews.llvm.org/D41102
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
