> On Jan. 18, 2011, 12:09 p.m., Merov Linden wrote: > > indra/newview/lltexturecache.cpp, line 1595 > > <http://codereview.secondlife.com/r/90/diff/1/?file=413#file413line1595> > > > > validate_idx being used in a test later, it's not just for > > (validate_idx == 0) that the behavior will be different. I need to > > understand better what that idx is all about or you need to give a bit more > > explanation before I approve this diff. > > Aleric Inglewood wrote: > The debug setting CacheValidateCounter is set to 'next_id', which makes > clear what it's meaning is: namely, the id that we will check next time. > next_idx is a very local variable that is simply set to the value of > CacheValidateCounter plus 1, and then that value is stored to > CacheValidateCounter again for next time. > > validate_idx is the ID that is actually being tested this time. Hence, it > should be the value of CacheValidateCounter before we increase that. > > Obviously, those ID's should be in the range 0...255, which is garanteed > by doing a modulo 256 on next_id before writing it to CacheValidateCounter. > > The old code also increases validate_idx *before* it is used. That means > that it will be in the range 1...256, and 0 is never tested (note that > validate_idx is just increased, there is no modulo applied, so it's obvious > that it shouldn't be increased). Even the wording ("next"_id) suggests that > validate_idx should just be the value of CacheValidateCounter, which is the > value of the previous next_id... > > So, after this patch, we get to the following code with validate_idx in > the range 0...255, as it should be. > > > Cron Stardust wrote: > Just double checking, as switching from pre-increment to addition can > change behavior: In both cases next_idx will have the same value after this > line is executed, however validate_idx will not. > > Prediff start state: validate_idx == 0; > Prediff stop state: validate_idx == 1; next_idx == 1; > > Postdiff start state: validate_idx == 0; > Postdiff stop state: validate_idx == 0; next_idx == 1; > > And another round over at the other end: > > Prediff start state: validate_idx == 255; > Prediff stop state: validate_idx == 256; next_idx == 0; > > Postdiff start state: validate_idx == 255; > Postdiff stop state: validate_idx == 255; next_idx == 0; > > So, yes, validate_idx will only have a 255 in it in this last case, > however the over-all behavior has changed: validate_idx isn't being > incremented at all in this line. > > WARNING: I have NOT looked at the rest of the diff, only at this one > line. Nor do I know if validate_idx should or shouldn't be incremented by > this line of code.
Given the way this seems to work to me without changing the sequence things are checked... I'd change line 1619 to if (uuididx == (validate_idx % 256)) Otherwise it was checking for 1 thru 256 and never 0... this does not change what appears to be incorrect coding where Aleric pointed out and thus won't change the current logic that it starts by checking 1 thru 255 before checking 0. To retain the current sequence that things are checked, you would have to compare uuididx to next_idx along with the change Aleric provided. However it seems to me that all is ok with using Aleric's correction and leaving the remaining code untouched. (I can't see where changing the sequence of checking makes a difference.) - Twisted ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/90/#review191 ----------------------------------------------------------- On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/90/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2011, 1:02 p.m.) > > > Review request for Viewer. > > > Summary > ------- > > Trivial patch, just removes stupidity. > > > This addresses bug VWR-24321. > http://jira.secondlife.com/browse/VWR-24321 > > > Diffs > ----- > > doc/contributions.txt b0bd26c5638a > indra/newview/lltexturecache.cpp b0bd26c5638a > > Diff: http://codereview.secondlife.com/r/90/diff > > > Testing > ------- > > > Thanks, > > Aleric > >
_______________________________________________ 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
