[PATCH] D33155: [DWARFv5] Support FORM_strp in the line table header

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D33155#759961, @probinson wrote:

> FYI I'm away at a conference next week and I might not get back to this until the week after.






================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:58-66
+    void Init(const DWARFUnit *U);
+    void Init(const DWARFUnit *U, const RelocAddrMap *RMap, uint16_t Ver,
+              uint8_t AddrByteSize, bool IsDwarf64) {
+      Unit = U;
+      RelocMap = RMap;
+      Version = Ver;
+      AddrSize = AddrByteSize;
----------------
dblaikie wrote:
> I'd consider keeping the ctors, dropping these Init functions, and where you have:
> 
>   x.Init(...);
> 
> use:
>  
>   x = FormValueInfo(...);
> 
> perhaps? since it's simple to copy/move, etc. Seems like less API surface, surprises (no/less chance that Init leaves some stale state, etc), etc :)
Still curious about this.

Also this class doesn't seem to provide any encapsulation, so it's weird that it has init/ctor like behavior - since there's no invariants that can be held, etc. Should it have a narrower surface area? Or maybe wider - perhaps a couple of functions that return this struct? With names that make clear their use case (the two different ways of using this function that represent different situations, etc)? Will those use cases be unified in some way?

Sorry, I'm still a bit fuzzy on the details/goals/end state.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:86
   ValueType Value;              // Contains all data for the form.
-  const DWARFUnit *U = nullptr; // Remember the DWARFUnit at extract time.
+  FormValueInfo Info;           // Info useful during extraction.
 
----------------
probinson wrote:
> dblaikie wrote:
> > The other point I was making here was that maybe the members of FormValueInfo could be members of DWARFFormValue directly.
> > 
> > I see there are a few cases where FormValueInfo objects are constructed separately from a DWARFFormValue - but they're all /inside/ a DWARFFormValue member function if I'm not mistaken. Why are those cases not using the FormValueInfo in the DWARFFormValue they're a member of?
> Those cases are static member functions.
> 
> it sounds like you would prefer each method to have several overloads for the different use-cases.  I was thinking that FormValueInfo could be factored out into an independent class, constructed by the caller and then passed in, reducing the DWARFFormValue API surface (as there could be simply one copy of skipValue and getFixedByteSize instead of several of each).
I'm not sure what I'd prefer - mostly trying to understand the current motivations/use cases/etc. Thanks for pointing out/explaining the details.

With regard to one function that takes a struct and behaves differently depending on the struct, versus two functions that behave differently/have different parameters:

I'd tend to prefer not to make a decision dynamic when it's static (ie: if all the callers know, statically, which behavior they want - no need to make the caller do dynamic work (yeah, the inliner can fix this, but from a legibility standpoint too, it seems simpler)). But this doesn't always apply - if the callee does mostly common work and somewhere deep inside does one of two different things based on those callers, then yeah, it may make sense to have a single function with some kind of dynamic parameter.


https://reviews.llvm.org/D33155