[opensource-dev] Review Request: make PREHASH variables char const* const

[Boroondas Gupte]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110120/ce87c419/attachment-0001.htm