#425 closed task (fixed)
CParamNode should not return NULL pointers
Reported by: | Philip Taylor | Owned by: | André Puel |
---|---|---|---|
Priority: | Nice to Have | Milestone: | |
Component: | UI & Simulation | Keywords: | simple |
Cc: | Patch: |
Description
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.
Attachments (2)
Change History (10)
comment:1 by , 14 years ago
Keywords: | review added |
---|---|
Resolution: | → worksforme |
Status: | new → closed |
comment:2 by , 14 years ago
Resolution: | worksforme |
---|---|
Status: | closed → reopened |
comment:3 by , 14 years ago
comment:4 by , 14 years ago
Keywords: | review removed |
---|
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.
by , 14 years ago
by , 14 years ago
comment:5 by , 14 years ago
Keywords: | review added |
---|---|
Owner: | changed from | to
Status: | reopened → new |
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".
comment:6 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:8 by , 8 years ago
Keywords: | review removed |
---|
I closed the ticket, my bad.
Reopened it now.