Expand C.9: (Minimize exposure of members) for member functions

[fekir]

Currently C.9 explains mainly how to handle member variables, but says nothing about member function.

In particular I would like to add something like

##### Note

Private non-virtual member function do expose implementation detail of the class.
To avoid exposing those implementation detail, consider moving the implementation to the corresponding implementation file in an anonymous namespace.

And as example something like

header file (before)

#include <optional>

class C{
int data;
// ...
private:
std::optional<int> fun(); // optional used only here(!)
}

impl file (before)

std::optional<int> C::fun(){
  return this->data == 42? std::nullopt : this->data;
}

and

header file (after)

class C{
int data;
// ...
}

impl file (after)

#include <optional>
namespace{
  std::optional<int> fun(int data){
    return data == 42? std::nullopt : data;
  }
}

and as enforcement "flag private non-virtual member functions"

Benefits:

• smaller header files, it is easier to recognize what the class does (for end-user)
• generally means more stable API/ABI
• faster build times, as potentially less includes and code to parse
• potentially smaller binaries (especially if class is exported)
• different optimization opportunities (especially if a class is exported from a library), with the second approach, compilers and tools for static analysis can diagnose much more easily if fun is dead code or not.
• makes easier to set invariant, as those are normally set by the interface (public/protected functions), and it ensures by design that private member functions can not call public functions

[BenjamenMeyer]

So you're advocating the PIMPL method - that's useful for hard ABI requirements; but it's not the only method.

For instance, you could achieve the same effect by having a public interface that only defines the public side and then a private implementation that derives from the public interface. This is actually cleaner than the PIMPL method, which usually ends up littering the code with things like P & Q pointers for which portion (public or private) of the object you want to work on; it also makes it easier for debuggers, it just requires that you separate the code and headers a bit more:

1. Public Header that defines the public interface
2. Private Header that creates the actual object derived from the public interface
3. Private implementation that implements the private definition

Done right you have very easy source compatibility across versions and can easily change out the underlying implementation or even relink without having to worry about what is using the public interface.

So what's my point here as relevant to this repo?

There's multiple paths that are equally viable that solve the same problem. Which one you choose is really based on how you want to work, etc. Therefore this is probably something that shouldn't be in the guidelines, at least not unless you want to have a section discussing all the different equally viable methods which if I'm not mistaken is beyond the scope of these guidelines.

[MikeGitb]

So you're advocating the PIMPL method - that's useful for hard ABI requirements; but it's not the only method.

I don't see what this has to do with pimpl, as there is no pointer (and also no dynamic allocation).

That being said: using local helper functions instead of private member functions is certainly a valuable tool, but a general ban on private memberfunctions seems far too strict for me. At least in a non-module world, where a lot of public functions are implemented in the header anyway (templates, header only libraries or just in order to facilitate inlining) and consequently the helpers must be available in the header anyway.

[BenjamenMeyer]

So you're advocating the PIMPL method - that's useful for hard ABI requirements; but it's not the only method.

I don't see what this has to do with pimpl, as there is no pointer (and also no dynamic allocation).

The two namespaces have to integrate in some manner. Whether that's using the Pointer method like PIMPL does or a similar non-pointer method, it's still the same basic pattern of switching between internal objects based on what part of the object (public vs private) you're working on.

That being said: using local helper functions instead of private member functions is certainly a valuable tool, but a general ban on private memberfunctions seems far too strict for me. At least in a non-module world, where a lot of public functions are implemented in the header anyway (templates, header only libraries or just in order to facilitate inlining) and consequently the helpers must be available in the header anyway.

I do agree. Personally, I use a mix of public, protected, and private methods - even friending on a rare occassion - and find them all useful to use. Not everything should be public; nor should every non-public thing be protected.

[MikeGitb]

The two namespaces have to integrate in some manner. Whether that's using the Pointer method like PIMPL does or a similar non-pointer method, it's still the same basic pattern of switching between internal objects based on what part of the object (public vs private) you're working on.

There is no internal object. The op is talking about private functions not private data.

[fekir]

@BenjamenMeyer

So you're advocating the PIMPL method - that's useful for hard ABI requirements; but it's not the only method.

No, look at my examples again, there is no pointer to any implementation, just a smaller header file that does not expose some implementation details, but also does not change the semantic of the class (if part of class hierarchy, if it has values/move/copy semantic, ...) nor adds any indirection (a pointer to some implementation) or increases memory usage (no additional allocation)
Check again the enlisted advantages, some of those might be irrelevant, but I'm sure that generally one or two are more or less relevant for many projects (especially build times, without the need to froward declare).

PIMPL is useful for hiding (mutable) data, I'm talking about functions.

The point you make have little to do with the proposed addition, and the suggested approach requires much more code, indirections, and the final class has probably not value semantic, or requires additional memory.

@MikeGitb

That being said: using local helper functions instead of private member functions is certainly a valuable tool, but a general ban on private member functions seems far too strict for me.

"Ban" is a big word, C.9 does not currently ban every class that does not use PIMPL, or has public member functions.
If you read the note I've proposed to add, I would describe it as "motivated recommendation" ;).

In some cases (templates, friends, constexpr, header-only library, ... and as you mentioned, aggressive inlining) it is not even possible or desirable to move private functions to implementation files.
But I believe, as C.9 suggests, that it is better to first hide the implementation detail, if this is not desireable (want more inlining/header-only library) or possible (constexpr, template, ...) then make it "public", as in visible in the header file, with a private member function.

[BenjamenMeyer]

So you're advocating the PIMPL method - that's useful for hard ABI requirements; but it's not the only method.

No, look at my examples again, there is no pointer to any implementation, just a smaller header file that does not expose some implementation details, but also does not change the semantic of the class (if part of class hierarchy, if it has values/move/copy semantic, ...) nor adds any indirection (a pointer to some implementation) or increases memory usage (no additional allocation) Check again the enlisted advantages, some of those might be irrelevant, but I'm sure that generally one or two are more or less relevant for many projects (especially build times, without the need to froward declare).

PIMPL is useful for hiding (mutable) data, I'm talking about functions.

The point you make have little to do with the proposed addition, and the suggested approach requires much more code, indirections, and the final class has probably not value semantic, or requires additional memory.

PIMPL is one method. It doesn't necessarily have to do with data; it has to do with functional separation between public and private APIs. PIMPL itself uses pointers (typically name P & Q); but you could achieve the same result with a static allocation of another class object - so you don't necessarily have to dynamically allocate memory to achieve the goal.

Conversely, I also mentioned using separate private and public headers for different definitions, abstract classes, inheritance, etc - another way to skin the hypothetical cat. This also doesn't necessitate dynamic allocation of memory.

The method mentioned in the original message discusses using anonymous namespaces to achieve something like (or a variation on) PIMPL (not called out, but the same effect in the end).

My point still stands, however, that there are multiple, valid approaches to solving the problem of hiding some implementation details - method and/or data, take your pick; and I'm not sure the guidelines should be favoring one method or another. Each has its trade-offs, usages, etc.

I'll be quiet now, and let the editors make their call.

[fekir]

PIMPL is one method. It doesn't necessarily have to do with data; it has to do with functional separation between public and private APIs.

Well, if there is no data... then probably there does not need to be any pointer to anything, or am I missing something?

The method mentioned in the original message discusses using anonymous namespaces to achieve something like (or a variation on) PIMPL (not called out, but the same effect in the end).

Nitpicking:
The main point was not exposing the implementation detail in the header at all, because it is not necessary, unlike member variables. Anonymous namespaces are not strictly necessary.

In C this "pattern" (I'm not even sure it should be called pattern as it is such an obvious property, it's the main reason we have header and implementation files) is (in my experience) followed more strictly.
In C++, because of private (and maybe because other languages with classes and private do not have separate header and implementation files) the tendency is to put everything unnecessarily in the header file.

My point still stands, however, that there are multiple, valid approaches to solving the problem of hiding some implementation details - method and/or data, take your pick; and I'm not sure the guidelines should be favoring one method or another. Each has its trade-offs, usages, etc.

The guidelines have PIMPL as concrete suggestion.

PIMPL is for solving the issue that a class normally needs it member variables to be exposed, does not matter if public or private, and PIMPL is a technique for avoiding such exposure when it is problematic.
For private functions this is a non-issue; why add the private functions in the header file, as it is not required?
No need for advanced techniques, there is no problem to solve (unless in certain situations, like constexpr, templates, ...).

Also this "technique"/"pattern" does not rule PIMP or your suggestion out, and viceversa, your suggestion and PIMP do not rule this technique out.

[coffee-lord]

An alternative pattern that I've come to love is what some people call an "Impl pattern". The basic idea is that you declare a dummy "Impl" class in the scope of your class thus giving it access to private variables. And then write static member functions i.e.

// Header

class Foo {
public:
  void foo();

private:
  struct Impl;
  int data;
};

// Implementation

struct Foo::Impl {
  static std::optional<int> fun(Foo& self) {
    return self.data == 42 ? std::nullopt : self.data;
  }
  // Other private functions...
};

void Foo::foo() {
  const auto r = Impl::fun(*this);
  // ...
}

This pattern requires some boilerplate code, namely having to type self. to access member variables/functions, but otherwise I'd say it does a pretty good job at hiding implementation detail and keeping the overall code structure similar to actual private member functions.

Just my 2c

[fekir]

@coffee-lord

Thanks for the suggestion, I'm aware of this pattern.
Except for the hint with an anonymous namespace, I do not think that the suggested wording rules this pattern out, does it?

[coffee-lord]

No, the suggested wording doesn't rule this pattern out. However, I agree with Mike in that this note seems a bit too situational to be included in the guideline. I guess you could put it under "motivated recommendation" but historically that's not how people interpret C++ Core Guidelines, unfortunately.

Personally I don't use this pattern to hide implementation details per se but rather to avoid the unnecessary recompilation on my potato laptop. I don't really have an opinion on whether private member functions should or should not be exposed.

Maybe the note should say something along the lines of "hey if you don't want to expose private member functions you can do this or that"?

[MikeGitb]

"Ban" is a big word, C.9 does not currently ban every class that does not use PIMPL, or has public member functions.
If you read the note I've proposed to add, I would describe it as "motivated recommendation" ;).

I was mostly referring to your suggested enforcement section.

[fekir]

@MikeGitb

"Ban" is a big word, C.9 does not currently ban every class that does not use PIMPL, or has public member functions.
If you read the note I've proposed to add, I would describe it as "motivated recommendation" ;).

I was mostly referring to your suggested enforcement section.

I see.
I thought the enforcement section describes how to enforce the rule, if you want to enforce it

@coffee-lord

Personally I don't use this pattern to hide implementation details per se but rather to avoid the unnecessary recompilation on my potato laptop. I don't really have an opinion on whether private member functions should or should not be exposed.

And this is one very good reason why I believe it should be part of the guidelines.
No-one likes long recompilation times, or the need to recompile if an implementation detail changed.

Why would you want to pessimize your experience?
Why would you want to add the implementation detail to the header (excluded when you have too)?

[MikeGitb]

I thought the enforcement section describes how to enforce the rule, if you want to enforce it

But thats just it: Stated as is, with the only exception of virtual functions, I can't think of a single project where I would apply it and I can't believe I'm the exception in this respect. At the very least I would like to see this check limted to header files and have an exemption for constexpr member functions and member functions of templates.

AFAIK (I'm not a CG Maintainer) the CG are explicitly not meant to be a "pick-and-choose" collection of guidelines (although they imho sometimes feel like it). By default you should be able to apply all checks to a (new) project and not have to go through each and every one of the guidelines and have to decide (worst case by committee) "Is this useful for my project? Is this? And that?". Of course there will alsways be project specific list of checks that wil be disabled, but imho the guidelines shoul strive to be written in a way that those are as small as possible.

Just to be clear: Contrary to some other posters in this thread, I'm not opposed to the recommendation in the note itself. "Avoid" and "Consider" are sufficiently strong qualifiers to make it clear this is not a hard universal rule. And contrary to some of the alternatives that have been mentioned here, the suggested alternative is simple and straight forward to apply and understand.

[fekir]

@MikeGitb

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c9-minimize-exposure-of-members states that it is a good thing to hide implementation details, and explains that using a private member variable is, without too much effort, too many headaches and overhead, the simplest and most straight-forward solution.

I would like to have a guideline for functions too, but the guideline for data is not the simplest and most straight-forward solution for functions.

Just like C.9 says in a note that protected data is bad, I would be completely satisfied to have a note (not a separate guideline) about it.

[MikeGitb]

@fekir : As I said:

I'm not opposed to the recommendation in the note itself.

I'm just against the enforcment part in the original post.

Alternative would be something like:

Enforcment:
Flag private member functions, except

• virtual functions
• functions that have to be defined inline anyway (e.g. member functions of class templates, constexpr)
• Special member functions (e.g. private constructors)
• ... (there are probably a few more exceptions that don't come to me right now)

[fekir]

Looks good to me

[fekir]

The same point obviously holds for free functions and classes; do not put them in the header file if those are implementation details unused by the users (unless constexpr, blah blah, same list of exceptions).

Fortunately with free function and classes I do not see this anti-pattern that often.

[fekir]

A talk that basically proposes the same I wrote (from 2017)
Klaus Igleberger, "Free your functions": https://www.youtube.com/watch?v=nWJHhtmWYcY

In particular Encapsulation (after ~5 min), but I think the whole talk is worth a watch

[hsutter]

Editors call: The problem seems rare and specialized. And it's not clear why an anonymous namespace is needed, just defining the function out of line removes the dependency (just using the type's name in a signature doesn't require a definition).

[fekir]

@hsutter A couple of question to understand the process better

it's not clear why an anonymous namespace is needed

Why not ask before closing?
The anonymous namespace is not necessary (also written in a comment).
Reducing visibility at source code level has advantages, so has also reducing visibility of unneeded symbols.

The anonymous namespace tells the reader that the function is private/that it is used only in the given translation unit, and it avoid exporting it by accident (unlike with MSVC on Windows, with GCC and Clang all symbols are exported by default)

The problem seems rare and specialized

Is there some data that indicates that private (non constexpr/virtual/templated and non special) member function are rare?
It seems to me that private functions are as common as public functions and private member variables, thus a guideline for member functions seems as justified as a guideline for member variables.

[BenjamenMeyer]

The anonymous namespace tells the reader that the function is private/that it is used only in the given translation unit, and it avoid exporting it by accident (unlike with MSVC on Windows, with GCC and Clang all symbols are exported by default)

static functions inside a file can achieve the same result, is cleaner, and can be easily promoted to "public"/shared without lots of rework/namespace changes/etc.

[pele1410]

My two cents as a random developer. The overall intention of this change seems valid and similar to the current rule on data. If you don't need to expose something, don't.

Are the maintainers against the spirit of the change? Or the change itself? Would a better-worded request be approved (something that doesn't mention anonymous namespacing)?

[fekir]

@BenjamenMeyer

This is going off-topic, as the anonymous namespace is just an implementation detail... but the anonymous namespace should be the preferred way (static is a too overloaded keyword, thus anon namespace is generally easier to explain/teach/...)

See also the corresponding core guideline:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#sf22-use-an-unnamed-anonymous-namespace-for-all-internalnon-exported-entities

What "lots of rework" does an anonymous namespace imply versus a static function? (But maybe we can move the discussion somewhere else, as it is not relevant for this issue?)

static void foo(){/* ... */}

bar(){
  foo();
}

vs

namespace{
 void foo(){/* ... */}
}

bar(){
  foo();
}

[cubbimew]

@pele1410 re "Would a better-worded request be approved" - pull requests are looked at first and pass more often, but it has to be convincing that it's addressing a problem dangerous enough or common enough to be a "core" guideline.

@fekir "guideline for member functions seems as justified as a guideline for member variables." we already have that guideline: C.4: Make a function a member only if it needs direct access to the representation of a class
and this issue's discussion appears to be on the details on how exactly this (pulling a member function out) is done, and that's what looked "specialized" on editor's call.

[fekir]

@cubbimew

Thank for pointing to C.4, but it does not address exactly the same issue, (or at least it seem to me).

The motivation for C.4 is that some function can change a given object with a subset of the provided interface, thus there is no need to give it access to all implementation details.
C.4 is talking about the API of a class, and arguing about free functions (that do not access the internal data) instead of member functions (that can access internal data).

A private (non-static) member function normally touches (reads or writes) some (or all) member data.
Thus according to C.4 it would be fine (also look at the enforcement).

But in practice (excluded the cases I mentioned), this is not necessary.
It is possible to rewrite the private member function as free functions, and it does not matter how the interface of the class looks like, because the function is used internally and member variables can be passed as normal parameters (ie the member functions I am talking about would generally not use the interface of the given class as C.4 explains).

C.9 seems to me a better place where to add such guideline because it talks about private members (functions or data) but provides concrete examples only for data.

I would also be happy if we can expand C.4.

Also C.4 is incompatible with the pattern used by @coffee-lord, which is compatible (except for the anonymous namespace) for the proposed guideline (enforcement would need rewording).
In his case all functions are still member functions, even if they do not touch any member variable, but I still find it better than putting all implementation details in the header file.

[BenjamenMeyer]

Since (and only since) you asked...

What "lots of rework" does an anonymous namespace imply versus a static function? (But maybe we can move the discussion somewhere else, as it is not relevant for this issue?)

static void foo(){/* ... */}

bar(){
  foo();
}

vs

namespace{
 void foo(){/* ... */}
}

bar(){
  foo();
}

Often in this case if you want you need to reference foo() using the full namespace. I've had lots of issues in the past with linkers hooking up functions in namespaces that were not fully qualified. Removing the namespace and using a simple static function resolves that since there is no namespace involved at all. If you have a namespace involved then all references need to get updated to the new namespace, etc. Granted if you want the function in a namespace you'd have to do that too when moving from a no-namespace usage to being in a namespace; but you could at least do that in separate steps without a major impact.

Further, it can then be easy to promote the function to public (just remove the static declaration and promote the function definition to a public header) and again, no namespace changes required - just how and where the function is declared (forward declaration).

Using the anonymous or implementation (or any other namespace you want to call it) has all these issues. Using a simple free function without C++ namespaces does not.

Don't get me wrong - I like C++ namespaces and use them extensively. But they do have their drawbacks too. Sometimes it's simpler to just not use them at all.