Opened 7 years ago

Closed 4 years ago

#8515 closed enhancement (fixed)

GCC behaviour in method chaining should be documented (easy)

Reported by: yourpalal Owned by: pdziepak
Priority: normal Milestone: R1
Component: Documentation Version: R1/Development
Keywords: doxygen Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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.

Change History (15)

comment:1 Changed 7 years ago by yourpalal

Summary: GCC behaviour in method chaining should be documentedGCC behaviour in method chaining should be documented (easy)

comment:2 Changed 5 years ago by nielx

Owner: changed from nielx to jscipione
Status: newassigned

comment:3 Changed 5 years ago by 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

comment:4 Changed 5 years ago by pdziepak

Owner: changed from jscipione 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.

comment:5 Changed 5 years ago by pdziepak

Resolution: invalid
Status: assignedclosed

comment:6 in reply to:  4 Changed 5 years ago by 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?

comment:7 Changed 5 years ago by pdziepak

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

comment:8 Changed 5 years ago by bonefish

Resolution: invalid
Status: closedreopened

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.

comment:9 Changed 5 years ago by 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!

comment:10 Changed 5 years ago by 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.

comment:11 Changed 5 years ago by 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.

comment:12 Changed 5 years ago by 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.

comment:13 Changed 5 years ago by 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?

comment:14 Changed 5 years ago by 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++);

comment:15 Changed 4 years ago by pulkomandy

Resolution: fixed
Status: reopenedclosed

Documented in hrev48626.

Note: See TracTickets for help on using tickets.