Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Use of pimpl pattern #1569

Open
dimitry-ishenko opened this issue Sep 19, 2024 · 3 comments
Open

Bug: Use of pimpl pattern #1569

dimitry-ishenko opened this issue Sep 19, 2024 · 3 comments

Comments

@dimitry-ishenko
Copy link
Contributor

dimitry-ishenko commented Sep 19, 2024

Observed Behavior

Every time peek into the Server innards, it makes me cringe a little. 😄 I think I've asked this before, but I don't remember what the answer was...

Why do we use pimpl pattern for the Server?

I understand when you have a library and you want to (1) hide implementation details from users, and/or (2) keep the ABI constant. That's when pimpl design comes in handy. Well, also (3) it helps reduce compilation time.

Expected behaviour

Neither (1) nor (2) applies to the Server though. Looking at the amount of headers included in the implementation files, I am not so sure (3) does very much either. Plus, there are other ways to mitigate compilation times.

Downsides of pimpl are pretty substantial:

  • Lots of boilerplate code.
  • Dynamic allocation.
  • Extra layer of indirection.
  • Massive code pessimisation (compiler can't inline or elide function calls).

I would expect the Server to use the usual OO pattern with no pimples (pun).

Steps to reproduce

  1. Open the Server code in your favorite editor.
  2. Observe the pimpl pattern being used everywhere.
  3. Cringe a little 😉

Environment

  • Server version: v2.4.x
  • Operating system: N/A
@Julusian
Copy link
Member

#1327

I don't have particularly strong feelings on this, other than I do find working with the boilerplate for methods rather tedious. And I don't feel confident enough to change what works and is already there

I do wonder how much more removing that would cause large recompiles.. It does 'leak' some implementation details to consumers of the headers, but in reality how often will they change without changes to the public methods..

@dimitry-ishenko
Copy link
Contributor Author

Good on finding that... I guess I should try using the search function next time. 😅

@dimitry-ishenko
Copy link
Contributor Author

#1327

I don't have particularly strong feelings on this, other than I do find working with the boilerplate for methods rather tedious. And I don't feel confident enough to change what works and is already there

I do wonder how much more removing that would cause large recompiles.. It does 'leak' some implementation details to consumers of the headers, but in reality how often will they change without changes to the public methods..

I will try to pick a small part (eg, const and mutable frame) and see what I can come up with... at some point...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants