search

epics-base / pvDataCPP

pvDataCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvDataCPP/
Other
6 stars 16 forks source link

Move getSubFieldT definitions out of class body #8

Closed dhickin closed 9 years ago

dhickin commented 9 years ago

Move definitions of 2 getSubFieldT overloads:

  1. shared_ptr PVStructure::getSubFieldT(const char *name) const
  2. shared_ptr PVStructure::getSubFieldT(size_t fieldOffset) const

out of function body so not implicitly inlined.

Implementation now uses stringstream. Moving outside class to avoid code bloat.

anjohnson commented 9 years ago

Review: Approve.

mdavidsaver commented 9 years ago

out of function body so not implicitly inlined. ... Moving outside class to avoid code bloat.

Afaik placement of the definition has no effect on whether a method is inlined. So this change is technically a no-op.

anjohnson commented 9 years ago

Afaik placement of the definition has no effect on whether a method is inlined.

It does, see https://isocpp.org/wiki/faq/inline-functions#inline-member-fns-more

mdavidsaver commented 9 years ago

Fair enough. In this case, I think we do want these methods to be in-linable in the non-error case. Since this is a non-template class, and creating the error messages doesn't involve the template parameter, the obvious thing to do is create a private method throwBadFieldType() or similar to handle the error case.

dhickin commented 9 years ago

I wanted to avoid a large function being inlined. I thought this would be an uncontroversial change. I think in practice the compiler is unlikely to actually inline this function though. It hadn't when I looked through objdumps (before the change).

Yes I could add a private function as you suggest or wait until we address an exception hierarchy for these functions, which I was planning to look at next.

So options are:

  1. Merge the pull request and address inlining later
  2. Add a private function to the pull request and mark function as inline
  3. Add a private function to the pull request and mark function as FORCE_INLINE
  4. Generate a new pull request with a private function, keeping the definition in the function body
  5. Generate a new pull request with a private function, keeping the definition in the function body, and mark function as FORCE_INLINE
anjohnson commented 9 years ago

If you're going to look at adding an exception hierarchy then this pull request will eventually become moot (since the original inline code will get smaller), so I suggest we put this request on hold for now, with the plan that eventually it won't be needed at all.