88
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?

Martijn Courteaux
  • 67,591
  • 47
  • 198
  • 287
  • 1
    Thanks, all! General conclusion: Don't do it, compiler will take care of it. – Martijn Courteaux Jun 10 '10 at 18:32
  • 2
    It depends. If you have them in the class definition, there is no need to inline them because they already are by default. If you are doing the implementation in a separate .cpp file, then it is up to the *linker*, which can be smart like on famous platforms or simply a dumb linker that won't inline anything on less know platforms AFAIK. – Khaled Alshaya Jun 10 '10 at 18:38
  • 4
    Let me add a few words to the answer I gave below. Personally, I dislike to clutter my class declaration with code very much as I consider this to be part of the (technical) documentation. Same argument for method definition in the header file, though not as bad. Ah and finally: You _really_ need getters and setters? :-) – mkluwe Jun 10 '10 at 18:48
  • 2
    @mkluwe +1 I agree, getters and setters is seldom a part of good practice. – daramarak Jun 10 '10 at 19:21
  • 2
    @daramarak: actually, it's mostly the setters that are bad practice. – André Caron Jun 30 '12 at 04:26
  • @AndréCaron A getter might not be bad practice (although I would favour push instead of pull) On the other hand getting the state of the object is a breach of encapsulation, that is in most cases bad practice. – daramarak Jul 03 '12 at 19:41
  • @damarak: exposing only a getter is not a breach of encapsulation, it only forces you to maintain *access* to the datum as part of your public interface. From the point of view of the client, there is no way to tell if the object owns the value or only has access (possibly through another object). Besides, following your argument, extracting any information from the object is a breach of encapsultion. – André Caron Jul 03 '12 at 20:02

11 Answers11

80

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.

JaredPar
  • 733,204
  • 149
  • 1,241
  • 1,454
  • 54
    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. – Mark Ransom Jun 10 '10 at 18:34
  • 23
    Infact, both GCC and VS offer inlining between source files. – Puppy Jun 10 '10 at 18:38
  • 7
    @Mark - that's sort of true since at that stage it's actually the linker, not the compiler. – Edward Strange Jun 10 '10 at 18:39
  • 2
    @DeadMG, I've never heard of that and can't imagine how it would be implemented. Got a link? – Mark Ransom Jun 10 '10 at 18:54
  • 3
    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. – mmmmmmmm Jun 10 '10 at 20:48
  • 1
    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 Jun 30 '12 at 05:38
  • 1
    I think LTCG will inline them – paulm Sep 13 '13 at 21:22
  • @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!) – underscore_d Aug 26 '16 at 09:26
32

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.

Kyle Strand
  • 15,941
  • 8
  • 72
  • 167
Khaled Alshaya
  • 94,250
  • 39
  • 176
  • 234
  • 4
    If the compiler wants to inline them… – mk12 Feb 19 '12 at 05:52
  • 5
    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. – Mooing Duck Jun 30 '12 at 05:39
15

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.

Greg Domjan
  • 13,943
  • 6
  • 43
  • 59
  • 7
    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? – stijn Jun 10 '10 at 20:10
  • 7
    @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. – David Thornley Jun 10 '10 at 20:39
  • 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 Jun 10 '10 at 20:40
  • 2
    David Thornley said it so much better than I. – Greg Domjan Jun 10 '10 at 20:41
  • 74
    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. – user168715 Jun 10 '10 at 21:34
  • 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. – Greg Domjan Jun 10 '10 at 21:59
  • 4
    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 – Steve Jessop Jun 10 '10 at 23:32
  • @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 Jun 11 '10 at 14:53
  • 3
    @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 Jun 11 '10 at 14:57
  • 1
    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. – Steve Jessop Jun 11 '10 at 16:51
11

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.

omerfarukdogan
  • 839
  • 9
  • 26
Edward Strange
  • 40,307
  • 7
  • 73
  • 125
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

Shane
  • 3,051
  • 3
  • 40
  • 45
  • 1
    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. – David Thornley Jun 11 '10 at 15:00
3

By putting the code in the header, you are exposing your internal class workings. Clients may see this and make assumptions on how your class works. This can make it more difficult to change your class later without breaking client code.

Tim Rupe
  • 4,323
  • 1
  • 22
  • 23
3

I gotta say, I don't have the strong aversion to this practice that others on this thread seem to have. I agree that the performance gain from inlining is negligible in all but the most heavily used of cases. (And yes, I have encountered such cases in practice.) Where I do this kind of inlining, I do it for convenience, and generally just for one-liners like this. In most of my use cases, the need to avoid recompilation on the client side if I ever change them just isn't that strong.

Yes, you can drop the inline, as it's implied by the placement of the implementation.

Also, I'm a little surprised at the vehemence against accessors. You can hardly sneeze at a class in any OO language without blowing a few down, and they are after all a valid technique to abstract implementation from interface, so it's a bit masochistic to claim them as bad OO practice. It is good advice not to write accessors indiscriminately, but I also advise you not to get carried away in the zeal to eradicate them.

Take that, purists. :-)

Owen S.
  • 7,665
  • 1
  • 28
  • 44
2

The code will compile slightly longer and you lose encapsulation. Everything depends on the size of the project and its nature. In most cases it's OK to make them inline if they do not have any complex logic.

BTW, you may skip inline if you implement directly in class definition.

Paul
  • 13,042
  • 3
  • 41
  • 59
2

The inline keyword is meaningless in your case

Compiler will inline your function if it can and wants to, regardless of keyword.

The keyword inline affects linking and not inlining. It is a bit confusing, but read up on it.

If the definition is in a different compilation unit (source file after preprocessor, basically) than the call, inlining will only be possible if whole project optimization and link time code generation are enabled. Enabling it greatly increases linking time (since it practically re-compiles everything in the linker), but obviously may improve performance. Not sure if it's on or off by default in GCC and VS.

Gal
  • 21
  • 1
1

I'd say that you don't need to bother with that. Read the FAQ section about inlining.

mkluwe
  • 3,823
  • 2
  • 28
  • 45
1

No need, start trusting the compilers, at least for such optimizations!
"But not always"

Numan
  • 3,918
  • 4
  • 27
  • 44