GCC behaviour in method chaining should be documented (easy)

[yourpalal]

As per ​this thread on haiku-commits, the behaviour of GCC in calculating parameters for a method chain can lead to confusing, hard to debug, errors. Since this is likely to come up when working with layout builders, we should put this in the api documentation, with a link from the builders' pages.

[korli]

Is this really GCC specific btw? From the standard, it looks like undefined behavior and implementation specific. ​http://stackoverflow.com/questions/21611215/gcc-bug-chaining-methods-broken-sequence-point

[pdziepak]

This ticket is invalid. The standard explicitly states that the order in which function arguments are evaluated is unspecified and may result in an undefined behavior. No valid C++ (or C) program should rely on any particular order as the compiler is allowed to change at any time.

[nielx]

Replying to pdziepak:

This ticket is invalid. The standard explicitly states that the order in which function arguments are evaluated is unspecified and may result in an undefined behavior. No valid C++ (or C) program should rely on any particular order as the compiler is allowed to change at any time.

Shouldn't this be the thing that is documented?

[pdziepak]

That depends on how much of the C++ standard we want to copy into our documentation.

[bonefish]

The C++ standard specifies plenty of cases of more or less obscure behavior and I doubt that even among seasoned developers many know them all. The evaluation order of function arguments might be one of the more popularly known one, but it still is not exactly obvious and I'm sure a good percentage of developers isn't aware of it. Combined with the fact that this is treated like a regular function argument, I think the percentage is even higher.

While I generally agree that we don't need to document such an C++ standard oddity, I think in this case we should. The layout builder API is intentionally designed to result in potentially long expressions that read like linearly processed statements. So the mistake of assuming linear evaluation is easily made and warrants a word of warning in the documentation IMO.

[yourpalal]

FWIW, I opened this ticket because this is behaviour that is easy to run into with the suggested use of the layout builders. Once I hit this, it was very difficult to debug because the code looks, inexplicably, as if it is executing out of order when you're in gdb. (Of course, it is executing in the order the compiler wanted, but not the order I wanted/was expecting)

Sacrificing the time of our API users on the altar of 'not my problem' seems silly to me. How many C++ programmers know the spec front to back? Even interpreting the spec can be tricky. GCC seems to treat the chained method calls as one single function call, where it prepares all arguments ahead of time, even though it looks to the programmer (or me, at least) like there are many sequential calls happening.

Haha, looks like Ingo beat me to the punch, but I will add my voice in anyway!

[pulkomandy]

Mentionning this (with a link to the relevant C++ spec section) in the documentation of the layout builder makes sense. This is similar to the beBook warnings when subclassing a BLooper with multiple inheritance (BLooper destructor doc here: ​https://haiku-os.org/legacy-docs/bebook/BLooper.html)

No need to go into great detail, just remind people the C++ rules apply in our API.

[nielx]

I don't understand the problem exactly, but if I am correct there is a risk in chaining methods, because it gives the impression it is a linear operation (like a chain), but the compiler does not (or does not have to) understand it like this.

Best thing would be to give an example within the context of the layout builders that looks logical and valid, but is not. We can briefly point out what to watch out for (and how to recognize that people have fallen in this trap), and refer to the specs for the whole story.

[pulkomandy]

One example of where this may be a problem:

BView* view1 = NULL;

BGroupLayoutBuilder(target)
    .Add(view1 = new BView())
    .Add(new MyView(view1));

MyView constructor may get either NULL, or the newly created BView.

[nielx]

That seems like quite an ... exotic way of writing code. Anyway, this example looks clear enough to use as example in the documentation. Where would we put it? Should it be in every layout builder, or is there a general overview doc?

[axeld]

A better example could look like this (note that BGroupLayoutBuilder is deprecated, anyway :-)):

int row = 0;
BLayoutBuilder::Grid<>(target) 
	.Add(viewA, row++)
	.Add(viewB, row++)
	.Add(viewC, row++);

[pulkomandy]

Documented in hrev48626.