Knut Anders Hatlen <[EMAIL PROTECTED]> writes: >>> * the private variable accessedIndexCols doesn't seem to be used any >>> more and could be removed >>> >>> * indexColRefItem (parameter to constructor) is also unused and >>> could be removed >> >> True. > > A related question: Could we also remove the index cols object from > the list of saved objects?
I think so. At least I have now removed it from the ActivationClassBuilder that's used in generate(...) But it cannot be removed from IndexToBaseRowNode since it is used for other purposes as well. > >>> * it would be good if the call to getCompactRow() in the constructor >>> had a comment saying that it will set the field compactRow as a >>> side effect. It could be a little confusing to readers that we >>> don't care about the return value of a getter. >> >> Hmm, yes maybe, but this is really an issue with the >> implementation of getCompactRow() in BasicNoPutResultSetImpl. >> compactRow is both returned and stored in a >> variable accessible to derived classes. And derived classes seem to be >> the primary users of this method. So I guess I would rather see >> BasicNoPutResultSetImpl fixed... but that did not seem to fit in the >> scope of this patch. > > I agree that this is an issue with the getCompactRow() implementation > and that it's outside the scope of this patch to fix it. But I think > it was easier to understand the old code since it explicitly set > compactRow. Just adding a very short comment would increase the > readability in my opinion: > > // sets the value of compactRow > getCompactRow(...); > > It could be only me, but when I see a getter called like a void > method, I immediately start wondering whether something is wrong. A > simple comment would assure me that everything is alright, and I > wouldn't have to spend time digging into getCompactRow() to find out > what's going on. :) I've added a comment that will be part of the next patch... > >>> * couldn't the work System.arraycopy() is doing be done as part of >>> the for-loop which follows it? Then we wouldn't have to go through >>> the array twice. >> >> Yes it could. I think I did it that way because the profiler showed that >> manual copy was expensive. I was hoping that arraycopy + writing nulls >> would be cheaper, but it is admittedly less intuitive. > > Well, I wouldn't say it is less intuitive. Your approach is probably > easier to read than if all the logic were inside the for-loop. Since > using standard library functions for such operations is a good thing, > and you also looked at it in a profiler, I have no objections to the > approach. Well, I have changed it now :) -- dt However, experience shows that for many people and many applications a dose of paranoia is reasonable - Bjarne Stroustrup
