[Lldb-commits] [PATCH] D39733: Simplify NativeProcessProtocol::GetArchitecture/GetByteOrder

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:104
 
-  virtual bool GetArchitecture(ArchSpec &arch) const = 0;
+  virtual const ArchSpec &GetArchitecture() const = 0;
 
----------------
eugene wrote:
> labath wrote:
> > eugene wrote:
> > > Why return reference instead of a value?
> > I'd actually reverse that: Why value instead of a reference ? :D
> > 
> > Returning by reference allows us to potentially avoid a copy. It does mean that the process object has to hold the archspec as a member, but that is currently not an issue, and we can always change that.
> >  Why value instead of a reference ? :D
> IMO returning by value is a default option, for 2  reasons:
> 1. References and pointers introduce implicit lifetime dependencies (prone to use-after-free)
> 2. Value can change over time, and client might not expect it to happen.
> 
> Potentially avoid a copy in place where a copy was made previously and didn't seem to hurt anybody seems like a bad trade-off for one more place where we can get a dangling pointer. 
> 
That's kinda true, but simply returning by reference does not force you to hold on to the reference everywhere. It just leaves the decision up to the caller -- if you put the result in a variable, you get a copy; if you put it in a reference (or just directly use it) then you don't. And if I see something like `const ArchSpec &arch = ...`, I will take a good look at what's the  lifetime of the object this refers to.

I'm not what you base your "default" statement on, but it certainly does not seem to be the default in llvm. Just look at llvm::Triple::str() -- it returns a reference to the string object private member.

ArchSpec is 56 bytes long (on x86_64), and it contains a string (potential heap allocation), so it's not exactly cheap to copy. GetByteOrder() is called on every register read, so it would be great to avoid copying an ArchSpec just to read a register. I'm not saying this will make register reading blazingly fast, but I think it's a step in the right direction.


https://reviews.llvm.org/D39733