failing unittest: multiple assignment to Node variable with references in document

[rhaschke]

I narrowed down my issue already mentioned in #567:
The failure is actually related to the assignment to the very same Node variable value.
If different Node variables are used each time, it works.
If no alias is involved, it works as well.

Please have a look to fix this issue. I already wasted a full day to narrow down that issue. Didn't look for reasons yet.

If I missed something and this kind of usage is illegal, please explain what's wrong.

[jbeder]

This is WAI, although it's definitely counterintuitive. I'm sorry you ran into trouble.

This is a consequence of nodes being treated as identity, so subsequent assignment is actually mutating.

I've pondered some ways to improve the API to disallow this, but haven't taken any action. See #208

[rhaschke]

IMHO it's not acceptable to keep an non-intuitive API. Obviously, I wasn't the only one yet, who tapped into this pitfall.
According to #208, I understand that you actually want to differentiate between assignment of a node value and assignment of a node alias. Why not making it explicit as follows:

Node a;
Node b;
a = 5;  // value of a should be changed
a = b;  // a.AssignData(b)
a = NodeAlias(b); // a.AssignNode(b)

The following commit realizes this behaviour and thus resolves the ambiguity. However, obviously it breaks the semantics of the existing API.

There is a third assignment method: a.reset(b). What is the difference of this w.r.t. the others? I don't see a need for a third semantics yet.
Finally, why do you need the m_isValid flag? Isn't the node automatically invalid, if m_pNode == nullptr?
Sorry for all these stupid questions. I just had a glance on the source code and these questions came to my mind...

[rhaschke]

@jbeder Obviously my new commits to this branch are not considered by github anymore, probably because you closed this PR. Do you mind to reopen it?

[jbeder]

I'll reopen this PR, but I'm not sure what you're trying to do; are you offering a patch that changes the API? Or are you trying to check in a failing test that highlights the aspect of the API that's at issue?

(Also, please consider your language when you write, "IMHO it's not acceptable to keep an non-intuitive API." This is an open source library maintained by one person (me!) in my spare time (which I don't have much of!). It's mean-spirited to declare that it's "not acceptable" for me not to devote significant amounts of time into change the API.)

[rhaschke]

Please consider your language.

I didn't want to offend you or your work on this project. I just stated that we shouldn't accept the current situation and with my recent commits, which became visible after re-opening this PR, I already suggested a potential solution that fixes the issue. I am myself maintainer of several actively used github projects and I really appreciate your work and your short reaction times!

Coming back to the discussion of the issue: Did you already had a look at my API changes? I'm sure I missed some details in the implementation - as I said I'm not very familiar with your source code. However, all the unittests pass and the semantic ambiguity of node and node alias is explicitly resolved. What do you think of this approach? I'm open for discussions on it.
Please, also shortly answer my questions in #568 (comment) so that I can gain a better understanding of your source.

[jbeder]

No worries, thank you for the apology.

To the point at hand: let me try to summarize your API change.

First change operator = so that

Node x = ...;
Node y = ...;
y = x;

just makes the variable y the same as x, but changes no internal state.

Then, add a new function NodeAlias, so that when you write

Node x = ...;
Node y = ...;
y = NodeAlias(x);

it will change internal state, so that y becomes an alias of x.

Is this summary correct?

If so, I think I do like the idea behind this change, although it is a breaking API change, so I'd like to think about it a little.

My instinct, however, is to actually remove the overload for NodeAlias (now that you're splitting it) and turn it into a function on Node, e.g., SetAlias. That seems clearer to me, and I didn't think of that before you suggested this patch.

What do you think?

[rhaschke]

Yes, I think this is essentially what this PR suggests. However, NodeAlias is not a function, but a class constructor.
Actually you already had two internal functions with these two different semantics: AssignData and AssignNode and the two assignment operators (for Node / NodeAlias as rhs) simply relay to them. Personally, I like the assingment version more than the function version, because it is more obvious what is assigned where.

[jbeder]

OK, let me think about these options.

(As for the function v. class constructor, I called it a function because of the syntax (which is all users see). I believe we'd rather have a free function that delegates to a constructor rather than making it a real constructor, as you have now; but no need to change this PR yet, since I'm not sure what syntax we want.)

[rhaschke]

May I ask my original questions again.

• What is the semantics of Node::reset(Node) in contrast to Node::AssignNode()?
• Why do you need the m_isValid flag? Can't you simply test for m_pNode != nullptr?