> 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. > > Twisted Laws wrote: > 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.)
> 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. That's the intention. Without the change suggested here, validate_idx ends up in the wrong range. See my more detailed review below. - Boroondas ----------------------------------------------------------- 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
