Is it good practice to make getters and setters inline?

Question

public:
     inline int GetValue() const {
          return m_nValue;
     }
     inline void SetValue(int nNewValue) {
          this -> m_nValue = nNewValue;
     }

On Learn C++, they said it would run faster. So, I thought it would be great to use on getters and setters. But maybe, there are some drawbacks to it?

Solution 1

I don't inline anything until a profiler has specifically told me that not inlining is resulting in a performance problem.

The C++ compiler is very smart and will almost certainly automatically inline such simple function like this for you. And typically it's smarter than you are and will do a much better job at determining what should or should not be inlined.

I would avoid thinking about what to or not to inline and focus on the solution. Adding the inline keyword later (which is not a guarantee of inline BTW) is very easy to do and potential places can be found readily with a profiler.

Solution 2

If you write them inside the definition, they are considered inline by default.

This means that they will be permitted in multiple compilation units (since class definitions themselves typically appear in multiple compilation units), not that they will actually be inlined.

Solution 3

This is Bad practice in public API's Any change to these functions requires recompilation of all clients.

In general having getters and setters is showing poor abstraction, don't do it. If you are constantly going to the raw data in another class then you likely need to re arrange your classes, instead consider how you wish to manipulate the data within a class and provide appropriate methods for doing so.

Solution 4

Negative points:

1. The compiler is free to ignore you.

2. Any change to these functions requires recompilation of all clients.

3. A good compiler will inline non-inlined functions anyway when it is appropriate.

Solution 5

I'd also like to add that unless you're performing millions of gets/sets per frame, it's pretty much irrelevant whether these are inlined or not. It's honestly not worth losing sleep over.

Also, keep in mind that just because you put the word 'inline' in front of your declaration+definition, doesn't mean the compiler will inline your code. It's uses various heuristics to work out whether it makes sense, which is often the classic trade off of speed vs size. There is however the brute force '__forceinline' keyword, at lease in VC++ (I'm not sure what it is in GCC), which stomps on the compilers fancy heuristics. I really don't recommend it at all, and besides once you port to a different architecture, it'll likely be incorrect.

Try to put all function definitions in the implementation file, and leave the pure declarations for the headers (unless of course you're template metaprogramming (STL/BOOST/etc), in which case, pretty much everything is in the headers ;))

One of the classic places people like to inline (at least in video games, which is where I'm from), is in math headers. Cross/dot products, vector lengths, matrix clearing, etc are often placed in the header, which I just think is unnecessary. 9/10 it makes no difference to performance, and if you ever need to do a tight loop, such as transforming a large vector array by some matrix, you're probably better off manually doing the math inline, or even better coding it in platform-specific assembler.

Oh, and another point, if you feel you really need a class to be more data than code, consider using good old struct, which doesn't bring the OO baggage of abstraction with it, that's what it's there for. :)

Sorry, didn't mean to go on so much, but I just think it helps to consider real world use cases, and not get too hung up on pedantic compiler settings (trust me, I've been there ;))

Good luck.

Shane

View more solutions

Comments

[Mark Ransom]

The compiler can't inline something if you put its implementation into a separate source file. Not relevant to the example presented, but worth mentioning nonetheless.

[Puppy]

Infact, both GCC and VS offer inlining between source files.

[Edward Strange]

@Mark - that's sort of true since at that stage it's actually the linker, not the compiler.

[Mark Ransom]

@DeadMG, I've never heard of that and can't imagine how it would be implemented. Got a link?

[stijn]

could you elaborate on the poor abstraction? what exactly do you mean, and how does it reflect to this example? should m_nValue just be public?

[David Thornley]

@stijn: A class should be an abstraction of, well, something. Its implementation shouldn't matter, and the operations on it should be meaningful to what it represents. There should be class invariants: statements that are always true about the class. Getters and setters expose the implementation almost as much as public variables. In my experience, the values of individual variables tend not to be meaningful in terms of the class abstraction, so you're allowing operations on the class that are not relevant to its abstraction. Getters and setters make it harder to keep invariants.

[Greg Domjan]

perhaps the stored value is an int, maybe it is some type that is freely created from an int and converted to an int, no idea. plucking the value from the class and doing something with it - what are you doing with it, why isn't that a feature of the class so that a getter is not necessary, try and minimize the low level linkage between classes.

[Greg Domjan]

David Thornley said it so much better than I.

[mmmmmmmm]

Maybe the linker could do the job if both files are in the same project. But if you link a pre-compiled DLL there is no way for it to inline anything that is not explicitly contained in the header files.

[user168715]

Hyper-dogmatic nonsense. You don't think there's a need to get the length of a string? Change the text on a UI button? Get the current coordinates of the mouse? Certainly it's possible to abuse getters and setters, as it is for any pattern. But "don't do it" as a blanket rule is IMO poor advice.

[Greg Domjan]

I feel it is a fair generalisation of a starting point. Given the level of the question is feels appropriate. I feel that your having a bit of an oversensitive backlash moment.

[Steve Jessop]

I think everyone's having an oversensitive backlash moment. Clearly containers have accessors (well, equivalent via references): it is explicitly part of their purpose to wrangle particular modifiable values. Also clearly, most classes are not containers. The problem with advice "in general, don't do it" is that it's hard to use the advice - to any example of a good getter/setter, the response is "I said don't do it in general, not don't do it when it's a good idea". So the advice is precisely equivalent to, "in general, only do things which are a good idea in the specific circumstances" ;-p

[David Thornley]

@user168715: Sure, there's a need to get the length of a string. This is an operation that's meaningful for a string, and it doesn't require that string length be directly stored in the class. Changing the text of a UI button is perhaps a better example, as the text is almost certainly a member variable, but it makes sense in the context of a UI button. You're giving examples of operations that make sense based on the class abstraction, but which are probably implemented as getters and setters. I don't see that we're actually disagreeing.

[David Thornley]

@Steve: Maybe we can come up with something more useful. Here's my proposal: Don't design in getters and setters. Design functions that are useful in terms of the abstraction. It makes sense, for example, to speak of the X coordinate of a point, or the length of a string. If these turn out to be, essentially, getters and setters, well, that's easy to implement.

[David Thornley]

Another comment here: if you force inlining, you're risking the possibility of making a loop too big, and possibly having some cache issues. Performance issues can be counterintuitive with modern CPUs. If you're going to force inline, do a performance test with and without, and keep the forced inline only if it helps.

[Steve Jessop]

Sounds good. Classes which represent something which naturally has visible state as part of the external interface, like the "subject" of an email message or the bgcolor of a desktop, are likely to end up with getters (and setters if the class is modifiable). It's wrong to design a class by wrapping members in getters and setters as a matter of course, just as much as it's wrong to make members part of the defined public interface as a matter of course.

[mk12]

If the compiler wants to inline them…

[Mooing Duck]

Correct, for DLLs it can't be inlined, that would defeat the purpose of a DLL. It can inline across static libraries though.

[Mooing Duck]

The C++ standard considers the functions inline if they're defined in the class definition, which is almost entirely unrelated to the compiler/linker inlining a function.

[paulm]

I think LTCG will inline them

[underscore_d]

@MarkRansom Sure, the compiler can't, but the linker can, and LTO is now a widely spread thing, which renders irrelevant the hypothetical performance objection to getters/setters (though there are other objections we can use in many situations!)