[PATCH] D34570: [DWARF] NFC: Collect info needed by DWARFFormValue into a helper

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 23 13:54:00 PDT 2017


dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Other than simplifying the DWARFFormParams a bit, this seems good to me.

(if DWARFAbbreviationDeclaration::extract is the only place that doesn't pass these extra params - maybe force that caller to construct a default DWARFFormParams, and remove the optionality from the primary API to avoid new callers neglecting to pass this important thing (& leave a comment there explaining that DWARF might need to be improved so that's supported? Or if it's supported & LLVM's parsing is missing functionality, a comment about that (about how parsing abbrevs can't be done without knowing the unit (& its address size, etc) that is specified there))

& shrinking the enum/passing by value seems fine by me in a followup if you like



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h:29-63
+class DWARFFormParams {
+  uint16_t Version = 0;
+  uint8_t AddrSize = 0;
+  dwarf::DwarfFormat Format = dwarf::DWARF32;
+
+  // DWARFUnit and DWARFDebugLine use this as a data member initialized while
+  // parsing the unit or line table, respectively.
----------------
This seems like a bit more encapsulation than I'd expect/seems warranted for a few parameters.

Especially the protected ctor, imho - it doesn't change any invariants of the class (the 3-arg ctor can be used to construct a default constructed object) & doesn't seem to protect from any really trivial misuse.

Also the 3-arg ctor would be about as good as braced init, and the value can be changed at any time with assignment anyway - maybe make this a struct? & leave the getRefAddrByteSize+getDwarfOffsetByteSize member functions for convenience - but I'd strip the rest of the accessors/ctors/friending out and make it a struct.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp:64
+DWARFFormValue::getFixedByteSize(dwarf::Form Form,
+                                 const DWARFFormParams *Params) {
   switch (Form) {
----------------
Should this be optional? The only place that doesn't pass it looks to be DWARFAbbreviationDeclaration::extract - is that usage correct? Does the abbrev table implicit_const not support the extra features? Should there be some error path/check for any forms that would require the params?


https://reviews.llvm.org/D34570





More information about the llvm-commits mailing list