On Thu, Sep 04, 2008 at 09:04:21PM +0200, Vincent van Ravesteijn - TNW wrote:
> >On Thu, Sep 04, 2008 at 09:36:45AM -0000, [EMAIL PROTECTED] wrote:
> >> @@ -68,6 +71,13 @@
> >> {
> >> all_formats_ = allFormats();
> >>
> >> + // Save the current selection if any
> >> + Format const * current_format = 0;
> >> + int const line = formatLW->currentRow();
> >> + if (line >= 0 && line <= formatLW->count()
> >> + && formatLW->selectedItems().size() > 0)
> >> + current_format = all_formats_[line];
> >
> >Are you sure that '<=' is better than '<' in this case?
> >If so, it might deserve a comment, '<=' looks rather unusual...
> >
> >Andre'
>
> It looks rather odd indeed, I did this merely because I 'sort of copied' it
> from elsewhere:
>
> GuiSendTo::applyView() (line 119):
> if (line < 0 || line > formatLW->count())
> ...
>
> bool GuiSendTo::isValid() (line 131):
> if (line < 0 || line > int(formatLW->count()))
> ....
>
> It is probably totally unnecessary to check whether line > formatLW.count(),
> because it will probably never happen.
>
>We have LASSERT for that as a sort of "non-lethal runtime-check".
>
>LASSERT(line >= 0 && line < formatLW->count(), return);
>
>and removing the 'if' is probably ok.
>
>Andre'
It looks like currentRow() might return -1 sometimes. It might even depend on
the platform you're on. If it is a 'normal' return value, I don't like to use
LASSERT for that. Elsewhere in the code it is explicity checked whether the
returned value is -1.
e.g. GuiPrefs.cpp : 1237
if (convertersLW->currentRow() == -1)
GuiTexInfo : 115
viewPB->setEnabled(fileListLW->currentRow() > -1);
Vincent
