Closed GoogleCodeExporter closed 9 years ago
Good catch.
Original comment by lieseg...@gmail.com
on 26 Sep 2013 at 3:38
Fixed in repo.
Original comment by lieseg...@gmail.com
on 27 Sep 2013 at 3:29
Your fix in the repo is not acceptable; it calls assert in debug mode, but
still triggers an array out of bounds in release mode.
Any number except 0 and 1 will trigger one of three conditions:
1. an immediate access violation, triggering "terminate" and causing the
program to hard abort
2. random memory to be pulled into a reference without a failure, causing
ticking time bomb to develop, making debugging essentially impossible
3. random memory to be pulled into a reference without a failure, but the
memory is sufficiently close that no failures happen, but strange and
inscrutable side effects to occur
None of these is acceptable. If you must return a reference, you should default
to some NullController, or return the first player controller. A pointer would
allow you to return NULL and prevent this issue, as would throwing an exception.
Furthermore, the assert(0) returns absolutely no information about what caused
the issue, making debugging harder. assert(0 && "Controller index out of
bounds") would be more acceptable.
Original comment by LoveOver...@gmail.com
on 27 Sep 2013 at 3:50
The assert was listed as an acceptable solution in your bug; combined with the
error message, it should be enough for a programmer to determine what happened.
Original comment by lieseg...@gmail.com
on 27 Sep 2013 at 3:58
To be more specific: the more thorough changes you suggest probably *would* be
better, but would require re-architecting things in a way I'm not comfortable
doing at this point in Angel's evolution. It's the kind of thing that could be
on the list for Angel 4, however, which I'm currently seeing as a big
refactor/cleanup as it will also be introducing multiple windows, networking,
etc.
Original comment by lieseg...@gmail.com
on 27 Sep 2013 at 4:11
The assert was listed as the lowest on a priority of four items, and does not
fully fix the situation in release; while it provides much more helpful than a
random undiagnosable crash, it is still not an acceptable solution on its own.
Restricting the domain of the function and/or returning an acceptable default
is a full solution, and is not that difficult of a change. If you do not wish
to change the interface, simply forcing controllerIndex to 0 would be
acceptable.
Original comment by LoveOver...@gmail.com
on 27 Sep 2013 at 4:15
Forcing the controllerIndex to 0 would still provide junk data if there were no
controllers plugged in.
The controller interface isn't optimal, for sure, but this is one of those
areas where we rely on client code to know what it's doing -- basically, the
callers of this function should be doing a check to ensure that they aren't
passing it an invalid index.
Original comment by lieseg...@gmail.com
on 27 Sep 2013 at 1:19
I understand the need to not break architecture, but the idea that it's not the
libraries duty to satisfy/check for reasonable pre and post conditions is one
I'd refer to jokingly as "morally reprehensible" (a phrase I'll surely use when
I show how messaging is broken).
The client code shouldn't need to dig through internals of objects to figure
out why it gets the wrong result or a crash from passing a perfectly reasonable
request in, even under different conditions (such as controllers not passed in,
etc). Especially not if a design goal is "easy to understand code".
Original comment by LoveOver...@gmail.com
on 27 Sep 2013 at 2:00
There's no need to dig through code -- it prints an error message telling you
what went wrong. It lets you make the decision about whether you want to keep
moving and focus on something else, or fix it right away.
Remember with all these things that Angel is a prototyping system first and
foremost. Certain errors are tolerable so long as the programmer is told about
them.
Original comment by lieseg...@gmail.com
on 27 Sep 2013 at 2:25
Original issue reported on code.google.com by
LoveOver...@gmail.com
on 21 Sep 2013 at 2:16