1

I had my doubts since I first saw where it leads, but now that I look at some code I have (medium-ish beginner), it strikes me as not only ugly, but potentially slow?

If I have a struct S inside a class A, called with class B (composition), and I need to do something like this:

struct S { int x[3] {1, 2, 3}; };
S *s;
A(): s {new S} {}
B(A *a) { a->s->x[1] = 4; }

How efficient is this chain: a->s->x[1]? Is this ugly and unnecessary? A potential drag? If there are even more levels in the chain, is it that much uglier? Should this be avoided? Or, if by any chance none of the previous, is it a better approach than:

S s;
B(A *a): { a->s.x[1] = 4; }

It seems slower like this, since (if I got it right) I have to make a copy of the struct, rather than working with a pointer to it. I have no idea what to think about this.

a concerned citizen
  • 787
  • 2
  • 9
  • 25
  • Is `s` a global pointer or `A`'s member? – wally Jan 08 '17 at 18:55
  • "t seems slower like this, since (if I got it right) I have to make a copy of the struct, rather than working with a pointer to it. I have no idea what to think about this." - that totally depends on the behaviour that you want to achieve. Do you want the instance of `struct S` to be shared between objects, or do you want each object of `A` to have its own `struct S` object? – Daniel Kamil Kozar Jan 08 '17 at 18:56
  • You might want to take a look at the Law of Demeter: https://en.wikipedia.org/wiki/Law_of_Demeter. I don't think there is a noticeable performance penalty for accessing members in a chain of `->`, but a better design would follow the principal of least knowledge. – E. Moffat Jan 08 '17 at 18:58
  • Is `B`'s constructor taking a pointer to an `A` then changing it, but `A` is not a member of the `B` being constructed? – wally Jan 08 '17 at 18:58
  • @Muscampester It's `A`'s private member (sorry for omitting that out). And, yes, I need `S` shared, `B()` may not be the only one calling `A *a`. – a concerned citizen Jan 08 '17 at 18:59
  • 1
    I was trying to convert the lines provided to small program to check on `https://godbolt.org/`, but there doesn't seem to be a coherent relationship between the classes. This is turning into a [bikeshed problem](http://meta.stackexchange.com/questions/31253/the-bikeshed-problem-and-stack-exchange) as everyone just answers about the general use of pointers. – wally Jan 08 '17 at 19:00

4 Answers4

2

The real cost of using pointers to objects in many iterations, is not necessarily the dereferencing of the pointer itself, but the potential cost of loading another cache frame into the CPU cache. As long as the pointers points to something within the currently loaded cache frame, the cost is minimal.

Tommy Andersen
  • 7,165
  • 1
  • 31
  • 50
  • `A *a` is actually created inside `B()` as a member variable, `A *a`, and instantiated in the constructor's list: `B(A *a): a {new A} ...`. Since I am working with `B()` most of the time, would this be good enough to fit into the cache? – a concerned citizen Jan 08 '17 at 19:02
  • @aconcernedcitizen actually `new A`, allocates memory to hold `A` somewhere on the heap, that may or may not be far from `B`, in essence they could be in two separate cache frames, which would not help. – Tommy Andersen Jan 08 '17 at 19:12
2

is it a better approach

In the case you just showed no, not at all.

First of all, in modern C++ you should avoid raw pointers with ownership which means that you shouldn't use new, never. Use one of the smart pointers that fit your needs:

  • std::unique_ptr for sole ownership.
  • std::shared_ptr for multiple objects -> same resource.

I can't exactly tell you about the performance but direct access through the member s won't ever be slower than direct access through the member s that is dereferenced. You should always go for the non-pointer way here.

But take another step back. You don't even need pointers here in the first place. s should just be an object like in your 2nd example and replace the pointer in B's constructor for a reference.

I have to make a copy of the struct, rather than working with a pointer to it.

No, no copy will be made.

roalz
  • 2,699
  • 3
  • 25
  • 42
Hatted Rooster
  • 35,759
  • 6
  • 62
  • 122
  • Come to think of it, even for `A *a` inside `B()` I have to instantiate `A`, somewhere. Then, more and more it looks like `X *x {new X}` is slower (and more prone to null pointer errors) than `X x;`. Did I get this correctly? – a concerned citizen Jan 08 '17 at 19:06
  • And, about the other concern, is it considered ugly code writing `a.s.x[1]`, or using even more levels of addressing? – a concerned citizen Jan 08 '17 at 19:24
0

Always avoid dynamic allocation with new wherever possible, as it is potentially a very expensive operation, and requires an indirection operation to access the thing you allocated. If you do use it, you should also be using smart pointers, but in your case there is absolutely no reason to do so - just have an instance of S (a value, not a pointer) inside your class.

  • So then the 2nd method would be preferable? Or even use a `const` reference to `A` in the argument of `B()`? Then it would look like `B(const A &a): { a.s.x[1] = 4; }`. Would instantiating the whole `struct` (not as a pointer) be faster (and preferable) than the pointer chain? – a concerned citizen Jan 08 '17 at 18:56
  • Yes. the second method is the best way to go. Accessing members via pointers is _not_ faster. –  Jan 08 '17 at 18:58
  • I realize this is not quite related to the OP, but I've seen in Qt that many use pointers instead of values, as in `return app->exec();`, instead of `app.exec()`. In the light of what has been ssaid so far, is the latter preferred? – a concerned citizen Jan 08 '17 at 19:12
0

If you consider a->s->x[1] = 4 as ugly, then it is rather because of the chain than because of the arrows, and a->s.x[1] = 4 is ugly to the same extent. In my opinion, the code exposes S more than necessary, though there may sometimes exist good reasons for doing so.

Performance is one thing that matters, others are maintainability and adaptability. A chain of member accesses usually supports the principle of information hiding to a lesser extent than designs where such chains are avoided; Involved objects (and therefore the involved code) is tighter coupled than otherwise, and this usually goes on the cost of maintainability (confer, for example, Law of Demeter as a design principle towards better information hiding:

In particular, an object should avoid invoking methods of a member object returned by another method. For many modern object oriented languages that use a dot as field identifier, the law can be stated simply as "use only one dot". That is, the code a.b.Method() breaks the law where a.Method() does not. As an analogy, when one wants a dog to walk, one does not command the dog's legs to walk directly; instead one commands the dog which then commands its own legs.

Suppose, for example, that you change the size of array x from 3 to 2, then you have to review not only the code of class A, but potentially that of any other class in your program.

However, if we avoid exposing to much of component S, class A could be extended by a member/operator int setSAt(int x, int value), which can then also check, for example, array boundaries; changing S influences only those classes that have S as component:

B(A *a) { a->setSAt(1,4); }

Stephan Lechner
  • 34,891
  • 4
  • 35
  • 58