Proposal for an exception to the enforcement of rule F.5 (local classes in source files)

[jansen-tiobe]

This issue has been submitted to me by [email protected].

The enforcement of rule F.5 is "Flag inline functions that are more than three statements and could have been declared out of line (such as class member functions).".

If you have a local class in your source file (.cpp), it is strange to have the member functions out of line. Can we relax the enforcement to:

"Flag inline functions that are more than three statements and could have been declared out of line (such as class member functions), except in case you have a local class in your source file.". I have added the "except ..." part.

Any thoughts?

[jwakely]

By "local class" do you mean one defined inside a function? If so, what has "in your source file (.cpp)" got to do with it? Wouldn't the exception apply to any local class, wherever it's defined?

Also the way you've phrased it ("you have a local class in your source file") makes it sound like any local class anywhere in a source file would turn off the enforcement for all functions throughout the source file, even in unrelated classes, which makes no sense.

Finally, it already says "could have been declared out of line" which is not the case for member functions of local classes, so the exception is already present.

[jansen-tiobe]

Hi jwakely,

Sorry for being unclear. Let me try to rephrase it. I meant a class defined in a source file .cpp as a helper class. The most natural way to do this is like:

x.cpp:

// this is a small helper class in a source file .cpp, only used within the .cpp file
class Helper {
  void m(int x) {
    x = 1;
    x = 2;
    x = 3;
    x = 4;
  }
};

whereas according to the rule F.5 enforcement it should be

x.cpp:

//  this is a small helper class in a source file .cpp, only used within the .cpp file
class Helper {
  void m(int x);
};

void Helper::m(int x) {
  x = 1;
  x = 2;
  x = 3;
  x = 4;
}

i.e. declaration and definition separately in the same file. According to Pepijn Kramer this is a bit redundant and pedantic if it concerns the same file.

[jwakely]

The point of the enforcement rule is "don't define functions as implicitly inline if they're not small functions that should be inlined" and whether the class is visible in other source files doesn't seem relevant.

[jwakely]

And to be clear, what you are describing is not a local class:

https://en.cppreference.com/w/cpp/language/class#Local_classes

[jansen-tiobe]

Thanks, so you mean that my second example is the way to go even if this happens in the same file? If you answer is yes, then we can close this ticket.

[jwakely]

I believe that's what the guideline says, and I don't see why it wouldn't apply equally to a class that isn't visible in other translation units. But the editors of the guidelines might feel differently.

[jansen-tiobe]

OK, let's wait for the verdict of the editors.

[bgloyer]

I don't think it is possible to have an enforcement for this rule. The rule is for a case of when to use inline but the current enforcement is for when not to inline. Some of the other rules have wording saying the rule is unenforceable. Should this one do that too?

It also seems like there are valid cases where an inline function could have more than three lines. The rule mentions templates but there are also header only libraries and classes defined in unnamed namespaces like the example above.

[bgloyer]

Also the rule seems to be a little out of date. If a small function is performance critical it should likely be constexpr in addition.

[jansen-tiobe]

I don't think it is possible to have an enforcement for this rule. The rule is for a case of when to use inline but the current enforcement is for when not to inline. Some of the other rules have wording saying the rule is unenforceable. Should this one do that too?

I completely agree with this and even raised a ticket for this mismatch between rule and its enforcement some time ago (#1730). It got rejected. I still think it is inconsistent and based on user feedback I can tell it is misleading if you implement the enforcement in a code checker and use the synopsis (which claims something else) to tell you that you violated this rule.

My proposal: I think that the current F.5 rule is not interesting. However, its proposed enforcement definitely is. I see people writing entire implementations in the class definition. That is what we need a coding standard rule for: to make sure maintainable code is produced. Once we have this adjusted F.5 rule, we can also list its exceptions such as:

• Template functions
• Helper classes that only occur in source files
• ...

What do you think? Should I make a proposal for such a rule (or replacement of F.5)?

[jwakely]

The bit about templates just seems wrong. Being defined in a header does not make them inline functions. They are treated similarly in terms of the one-definition rule, and become part of the ABI, but they're not actually "inline functions" unless defined with inline/constexpr/consteval, or in the class body.

[shaneasd]

I think the point was that being defined in a header means that the definition and declaration are in the same file so it's possible for them to be defined in the class body in a way that would not be for a conventional class member function with declaration in the header and definition in the cpp. Helper classes are an equivalent example where the declaration and definition are both in the cpp file so the function can be defined in the class body.

Whether they can be and whether they will or should be are all separate questions. I would argue that, in the absence of guidance, they will be, because authors will draw reference either from experience with other languages (many of which are structured in this way) or from other c++ libraries, notably the standard library. Looking at https://github.com/microsoft/STL/blob/main/stl/inc/xstring it appears that the convention (rightly or wrongly) is to always define member functions within the class body whether they are small or not. I think it is a reasonable assumption, given this, that, not knowing better, someone would write their template function with member functions defined within the class body and thus implicitly inlined.

This is explicitly mentioned in the core guidelines

Exception
Function templates (including member functions of class templates A::function() and member function templates A::function()) are normally defined in headers and therefore inline.

which seems inconsistent with the other example cases mentioned by @jansen-tiobe but also inconsistent with the enforcement.

Generally speaking, when reading the guidelines I gloss over the enforcement because I assume that this is written for the benefit of tools authors and doesn't add new information for readers trying to follow the guidelines. I can't see anything in https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#SS-force that makes me question this approach.

With that in mind, I would summarize the rule as "use the inline keyword to make sure things that should be inlined are inlined but don't bother for member functions defined in class body because they are implicitly inline" @jansen-tiobe 's Helper class would seem to obey this rule for either implementation (i.e. whether the definition of m is inside or outside the class definition.

The enforcement on the other hand effectively says (to me) "don't declare inline functions (implicitly or explicitly) unless they are short". That suggests that the first Helper example is bad and the second should be used which is the consensus reached in #2246 (comment) but I'm not clear on why that's the case and I don't think the guidelines as written offer any guidance on this question.