CParamNode should not return NULL pointers

[Philip]

CParamNode::GetChild returns pointers which could be NULL (if the child doesn't exist). In theory that should never happen (template validation ought to catch the errors first), but it's fragile and crash-prone.

Instead, it should return a dummy CParamNode object (by reference) with a bool Ok() method to allow explicit checking of whether the child really existed. GetChild should return another dummy object, ToInt/ToString/etc should log an error message and return 0/""/etc.

[Puel]

I closed the ticket, my bad.

Reopened it now.

[Philip]

Thanks! There's a few changes I'd like to see:

The nullCParamNode should be a statically-allocated global variable in the .cpp file. That should simplify the code a bit (no need to check if it's already been initialised or not), and would stop it getting reported as a memory leak. (IsNull can then just check against the address of the global.)

There should be tests that the null nodes generally work as expected - in test_ParamNode.h, add some lines to test that IsNull returns false for real nodes and true for null nodes, and that ToString returns an empty string on null nodes, etc. (Most of these are pretty obvious, but I like having tests anyway since it reduces the chance of accidentally breaking things in the future.)

I'd prefer IsNull to be negated and called something like IsOk or Ok or something, because I expect the common usage will be like

if (paramNode.GetChild("Anchor")->Ok())
  // do something with paramNode.GetChild("Anchor")->GetChild("Offset") or whatever
else
  // default/error handling

and that seems harder to misread than the double-negative of if (!foo->IsNull()).

Ideally, GetChild would return a CParamNode& instead of a CParamNode* (now that it will never return NULL), to simplify the usage syntax (GetChild("x").GetChild("y") instead of GetChild("x")->GetChild("y")). But that'll need lots of changes to the code that currently uses this class, so it's probably best to ignore that for now.

[Puel]

Here we go. I've done what you said.

By the way, I created a ticket about changing GetChild?()'s CParamNode* to CParamNode&: ​http://trac.wildfiregames.com/ticket/451 I will work in it later.

Note: Since trac didn't color the "patch2" file, I re-upload the file with the name "patch".

[philip]

(In [7302]) Make CParamNode not return NULL pointers, based on patch from Puel Fixes #425, #451

[anonymous]

Milestone Unclassified deleted