rnk added inline comments.
================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:29
+/// Carrying the size separately instead of trusting the size stored in the
+/// record prefix provides some extra safety and flexibility.
template <typename Kind> class CVRecord {
----------------
aganea wrote:
> aganea wrote:
> > To add to what you said in a comment above, do you think that if we could
> > add `assert(Data.size() == ((RecordPrefix
> > *)RecordData.data())->RecordPrefix + 2)` at relevant places below; and then
> > after a while we could simply switch to `RecordPrefix *`, once issues are
> > ironed out?
> I didn't mean now.
Eventually, yes, I think it's possible.
================
Comment at: llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h:56
+ RecordPrefix *Prefix = reinterpret_cast<RecordPrefix *>(&PreBytes[0]);
+ Prefix->RecordLen = 0;
+ Prefix->RecordKind = uint16_t(Sym.Kind);
----------------
aganea wrote:
> rnk wrote:
> > aganea wrote:
> > > Should be 2.
> > I could say 2, but this is the seralizer, so really it just gets
> > overwritten. Do you think I should leave it uninitialized, 2, -1, 0?
> I'd say "2" because you want as much as possible to keep data coherent in
> time. Regardless of what comes after. I think the code should be correct
> before being optimal. If someone looks for `RecordPrefix` and then tries to
> understand how it works, it'd be nice if the code displays the same, correct,
> behavior everywhere.
>
> But then, you can argue that `RecordPrefix.RecordLen` is simply a constrait,
> tied to the amount data that comes after. And maybe the calculation logic for
> that `.RecordLen` field should be hidden inside the `RecordPrefix` class. In
> that case, to avoid screwing the init order, ideally you could simply say:
> ```
> RecordPrefix Prefix{uint16_t(Sym.Kind)};
> CVSymbol Result(&Prefix);
> ```
> ...and let `RecordPrefix` (or `CVSymbol`) handle sizing?
Makes sense. I'll try to standardize on 2, and I like the logic that the data
becomes trustworthy.
================
Comment at: llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp:40
+ static CVSymbol Tombstone(
+ ArrayRef<uint8_t>(DenseMapInfo<uint8_t *>::getTombstoneKey(), 1));
return Tombstone;
----------------
aganea wrote:
> I suppose you've chosen 1 because that's a invalid record size? Comment maybe?
Actually, the reason I did this was to avoid ambiguity between the `T* begin,
T* end` ctor and the `T* base, size_t count` ctor. If I use zero, it's
ambiguous. Implicit null strikes again. :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60018/new/
https://reviews.llvm.org/D60018
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
