[PATCH] D46271: [CodeView] Improve debbuging of virtual base class member variables

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:793
 
+  /// \brief In inheritance, the pointer offset to virtual bases.
+  uint32_t VBPtrOffset;
----------------
aprantl wrote:
> bwyma wrote:
> > rnk wrote:
> > > bwyma wrote:
> > > > aprantl wrote:
> > > > > aprantl wrote:
> > > > > > rnk wrote:
> > > > > > > `vbptroffset` is pretty esoteric. It seems really unfortunate to have to widen the very popular `DIDerivedType` constructor to take this obscure parameter. Pointer and reference types are some of the most common kinds of DIDerivedType nodes, so taking an address space is less objectionable, even though it is specialized towards one kind of derived type node.
> > > > > > > 
> > > > > > > I'd recommend encoding this information in the `extraData` field, similar to how we encode bit offsets for bitfields, or making a new `DIInheritanceType` node. @aprantl, @dblaikie WDYT?
> > > > > > please drop the \brief.
> > > > > Seems reasonable; avoiding making DIDerivedType larger for all languages and platforms would be a good thing.
> > > > Although I believe DIInheritedType is a better solution, changing DW_TAG_inheritance from DIDerivedType to DIInheritedType is a substantial change which I would recommend making independently from this fix.  Furthermore, your valid argument regarding the esoteric nature of the field will be just as valid for DIInheritedType as it is for DIDerivedType, so I think in either case the field should be moved to extraData.  As a result, for the purposes of this patch I propose moving this field to the extraData field of DIDerivedType.  Is this sufficient?
> > > Yep, sounds good to me.
> > > please drop the \brief.
> > I have no personal attachment to this tag and I would be happy to remove it, but it matches the surrounding code and seems to be permitted by the [[https://llvm.org/docs/CodingStandards.html | coding standard]].  Can you explain why it should be removed?
> Sure! A few years ago we started enabling `autobrief` mode in LLVM's Doxygen configuration, which makes the use of \brief in the source code redundant. It's a distraction when reading the source code, so we shouldn't add it in new code and probably also remove it from existing code.
Given that I've seen you raise this comment a few times, I think it's probably worth doing a large pass to remove `\brief` across LLVM. Otherwise people will continue to copy it in an effort to fit in with existing code, and then we'll spend time on it during review.


https://reviews.llvm.org/D46271