----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/100/#review207 -----------------------------------------------------------
> Just one remark. In one test _PREHASH_AgentID is set to "AgentID" > (no doubt a place holder), while in the other you set it to 0 > (now needed because it's a const). Perhaps a more symmetric approach > is better? indra/llui/tests/llurlentry_stub.cpp <http://codereview.secondlife.com/r/100/#comment152> This test already contained these placeholder strings, so I just left it at that, only adapting the types. indra/newview/tests/llremoteparcelrequest_test.cpp <http://codereview.secondlife.com/r/100/#comment153> In this test here, however, I think the pointers actually ended up undefined in the original code, so setting them to NULL was the closest I possibility could think off. As building succeeded with this, I concluded that these pointers are passed around but never dereferenced during the test.[*] I feel setting nullpointers nicely signals that fact, though it should probably be augmented by a // never used during this test or // never dereferenced in this test comment to make that intent clear. I agree though, that we should try to handle this similarly in both tests, if possible. So I tried setting the pointers in indra/llui/tests/llurlentry_stub.cpp to NULL, too, which works nicely. However, I then realized that the tested code might have nullpointer guards around dereferencing code, so that if we pass nullpointers we are actually testing another scenario than when passing pointers to actual data and that this might be the reason why no null-pointer exceptions occur during the tests. Of course, we could now examine the tested code to see whether this is the case. However, it would be a bad idea to adapt the test code based on that examination, as the tested code might be changed later without this question being re-evaluated and the test rewritten accordingly. Is there another pointer value besides NULL (thus not usually tested for) that still fails reliably when tried to dereference? ---- [*] Remember that tests are executed when building and are hopefully completely deterministic, so that any potential null-pointer-exception would have shown up. - Boroondas On Jan. 18, 2011, 7:29 a.m., Boroondas Gupte wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/100/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2011, 7:29 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > For the reason for this change, see > https://jira.secondlife.com/browse/VWR-24487 and > https://jira.secondlife.com/browse/VWR-24522 > > What I did: > In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant > pointers to constants by search/replace. Then I tried to compile and added > const qualifiers in dependent code as needed to stop the compiler complaining. > > Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files > have been generated from the message template. If this generation wasn't a > one-off thing, the generating code must be changed, too, so it won't override > this change here when the generation happens the next time. > > > This addresses bug VWR-24487. > http://jira.secondlife.com/browse/VWR-24487 > > > Diffs > ----- > > doc/contributions.txt fc7e5dcf3059 > indra/llmessage/message_prehash.h fc7e5dcf3059 > indra/llmessage/message_prehash.cpp fc7e5dcf3059 > indra/llprimitive/llprimitive.h fc7e5dcf3059 > indra/llprimitive/llprimitive.cpp fc7e5dcf3059 > indra/llprimitive/llvolumemessage.h fc7e5dcf3059 > indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 > indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 > indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 > > Diff: http://codereview.secondlife.com/r/100/diff > > > Testing > ------- > > Compiled (standalone, 64bit Linux) with and without LL_TESTS. > Started the viewer, logged in, walked and flew around a bit. Everything seems > to work. > > > Thanks, > > Boroondas > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
