Re: [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/#review244
---


As already mentioned in the review request description, comments in 
message_prehash.h and message_prehash.cpp claim that these files have been 
generated:


indra/llmessage/message_prehash.h






indra/llmessage/message_prehash.cpp





Do we have to expect them to be re-generated again in the future? If so, the 
code for generating them should be changed alongside the change I propose here, 
so that future generations (pun intended) won't  inadvertently override and 
revert the change.

At first, I thought the code for generating them was not part of the source 
tree, but now I think I have located it in indra/llmessage/message.cpp. (I 
would have expected it to be a script, not a compiled program.) I will 
investigate this further and augment the review request with a change to the 
generating code, too, if applicable.

- Boroondas


On Jan. 20, 2011, 2:57 p.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/100/
> ---
> 
> (Updated Jan. 20, 2011, 2:57 p.m.)
> 
> 
> Review request for Viewer and Seth ProductEngine.
> 
> 
> 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.
> 
> 
> Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in 
> indra/llui/tests/llurlentry_stub.cpp and 
> indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually 
> are never dereferenced, even when not NULL, so that using NULL pointers 
> instead of place holder data won't change what code paths gets tested. Both 
> tests binaries executed without crashes and all the contained tests passed.
> 
> 
> 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

Re: [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/
---

(Updated Jan. 22, 2011, 7:40 a.m.)


Review request for Viewer and Seth ProductEngine.


Changes
---

Found and changed the generating code for message_prehash.(h|cpp), too.


Summary (updated)
---

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. Because this generation might 
not have been a one-off thing, I changed the generating code, too, so it won't 
override this change here when the generation happens the next time. However, 
that part of the code is not called by Viewer, although the relevant function — 
dump_prehash_files() — ends up in the Viewer binary. That function probably 
gets called by the simulator, when one runs the latter with -prehash. (See 
https://bitbucket.org/lindenlab/viewer-development/src/fc7e5dcf3059/indra/llmessage/message.cpp#cl-2532
 )


This addresses bug VWR-24487.
http://jira.secondlife.com/browse/VWR-24487


Diffs (updated)
-

  doc/contributions.txt fc7e5dcf3059 
  indra/llmessage/message.cpp 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 (updated)
---

Compiled (standalone, 64bit Linux) with and without LL_TESTS.
Started the viewer, logged in, walked and flew around a bit. Everything seems 
to work.


Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in 
indra/llui/tests/llurlentry_stub.cpp and 
indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually are 
never dereferenced, even when not NULL, so that using NULL pointers instead of 
place holder data won't change what code paths gets tested. Both tests binaries 
executed without crashes and all the contained tests passed.

Locally invoked start_messaging_system() with b_dump_prehash_file == true 
instead of FALSE, to see what would be generated after my change to 
dump_prehash_files().
The message_prehash.(h|cpp) generated by that had the correct type qualifiers 
and formatting, but some lines were removed or added compared to the modified 
files from the source tree. That probably means that the files aren't fully 
synchronized with the message template file in the source tree. Because the 
"added" constants are spread all over the file, while the "removed" ones were 
at the end, I'd wager that message_prehash.(h|cpp) in the viewer source tree 
are actually more up-to-date than the message template file in the source tree.


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

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

[Aleric Inglewood]

---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/100/#review245
---

Ship it!


Latest diff looks good. Great that you found the generator for this code!

- Aleric


On Jan. 22, 2011, 7:40 a.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/100/
> ---
> 
> (Updated Jan. 22, 2011, 7:40 a.m.)
> 
> 
> Review request for Viewer and Seth ProductEngine.
> 
> 
> 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. Because this generation might 
> not have been a one-off thing, I changed the generating code, too, so it 
> won't override this change here when the generation happens the next time. 
> However, that part of the code is not called by Viewer, although the relevant 
> function — dump_prehash_files() — ends up in the Viewer binary. That function 
> probably gets called by the simulator, when one runs the latter with 
> -prehash. (See 
> https://bitbucket.org/lindenlab/viewer-development/src/fc7e5dcf3059/indra/llmessage/message.cpp#cl-2532
>  )
> 
> 
> This addresses bug VWR-24487.
> http://jira.secondlife.com/browse/VWR-24487
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt fc7e5dcf3059 
>   indra/llmessage/message.cpp 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.
> 
> 
> Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in 
> indra/llui/tests/llurlentry_stub.cpp and 
> indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually 
> are never dereferenced, even when not NULL, so that using NULL pointers 
> instead of place holder data won't change what code paths gets tested. Both 
> tests binaries executed without crashes and all the contained tests passed.
> 
> Locally invoked start_messaging_system() with b_dump_prehash_file == true 
> instead of FALSE, to see what would be generated after my change to 
> dump_prehash_files().
> The message_prehash.(h|cpp) generated by that had the correct type qualifiers 
> and formatting, but some lines were removed or added compared to the modified 
> files from the source tree. That probably means that the files aren't fully 
> synchronized with the message template file in the source tree. Because the 
> "added" constants are spread all over the file, while the "removed" ones were 
> at the end, I'd wager that message_prehash.(h|cpp) in the viewer source tree 
> are actually more up-to-date than the message template file in the source 
> tree.
> 
> 
> 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