Hi again, thank you for the comments. Knut Anders Hatlen <[EMAIL PROTECTED]> writes:
> Would it be better to copy this logic in the new code? If I have > understood correctly, we get the same behaviour if we remove the outer > if in init(). Apart from the obvious benefit that we get an error if > this unexpected situation occurs, I also think it would make the code > easier to read. I could have sworn that I added that outer if-else at some point to avoid a "may not have been initialized" warning. However, I can now remove it without seeing that warning, so I'll do that. > I have looked more at the patch. It seems correct to me, and I think > it's a good change. Some minor comments about the changes in > IndexRowToBaseRowResultSet: > > * 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. > * 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. > * 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. -- dt
