Re: comctl32: Add missing break (Coverity)

[Nikolay Sivov]

On 2/23/2011 19:28, Amine Khaldi wrote:
> CIDs 1581 and 1583.
> @@ -1852,6 +1852,7 @@ COMBOEX_EditWndProc (HWND hwnd, UINT uMsg, 
> WPARAM wParam, LPARAM lParam,
>      case VK_UP:
>      step = -1;
> -
>    break;
>    case VK_DOWN:
>    /* by default, step is 1 */
>    oldItem = SendMessageW (infoPtr->hwndSelf, CB_GETCURSEL, 0, 0);

This is wrong.
> @@ -2297,6 +2297,7 @@ static LRESULT 
> TOOLBAR_Cust_AvailDragListNotification(const CUSTDLG_INFO *custIn
>              TOOLBAR_Cust_AddButton(custInfo, hwnd, nIndexFrom, nIndexTo);
>          }
>      }
> - break;
> case DL_CANCELDRAG:
>     /* Clear drag arrow */
>     DrawInsert(hwnd, hwndList, -1);

Why? Looks to me it's fine to clear on dropped case too. Coverity is a 
bit paranoid about missed breaks.

Also don't include completely unrelated changes to one patch.
> Regards,
> Amine.

comctl32: Add missing break (Coverity)

[Michael Stefaniuc]

Nikolay Sivov wrote:
> On 2/23/2011 19:28, Amine Khaldi wrote:
> > CIDs 1581 and 1583.
> > @@ -1852,6 +1852,7 @@ COMBOEX_EditWndProc (HWND hwnd, UINT uMsg,
> > WPARAM wParam, LPARAM lParam,
> >      case VK_UP:
> >      step = -1;
> > -
> >    break;
> >    case VK_DOWN:
> >    /* by default, step is 1 */
> >    oldItem = SendMessageW (infoPtr->hwndSelf, CB_GETCURSEL, 0, 0);
>
> This is wrong.
> > @@ -2297,6 +2297,7 @@ static LRESULT
> > TOOLBAR_Cust_AvailDragListNotification(const CUSTDLG_INFO *custIn
> >              TOOLBAR_Cust_AddButton(custInfo, hwnd, nIndexFrom,
> > nIndexTo);
> >          }
> >      }
> > - break;
> > case DL_CANCELDRAG:
> >     /* Clear drag arrow */
> >     DrawInsert(hwnd, hwndList, -1);
>
> Why? Looks to me it's fine to clear on dropped case too. Coverity is a
> bit paranoid about missed breaks.

A missing break is not trivial to detect; especially if something is
done in a specific case. That's why Wine has a ton of "/* fall through
*/" annotations. IMHO they should be added here too .

bye
    michael

comctl32: Add missing break (Coverity)

[Nikolay Sivov]

On 2/23/2011 19:56, Michael Stefaniuc wrote:
> Nikolay Sivov wrote:
> > On 2/23/2011 19:28, Amine Khaldi wrote:
> > > CIDs 1581 and 1583.
> > > @@ -1852,6 +1852,7 @@ COMBOEX_EditWndProc (HWND hwnd, UINT uMsg,
> > > WPARAM wParam, LPARAM lParam,
> > >       case VK_UP:
> > >       step = -1;
> > > -
> > >    break;
> > >     case VK_DOWN:
> > >     /* by default, step is 1 */
> > >     oldItem = SendMessageW (infoPtr->hwndSelf, CB_GETCURSEL, 0, 0);
> >
> > This is wrong.
> > > @@ -2297,6 +2297,7 @@ static LRESULT
> > > TOOLBAR_Cust_AvailDragListNotification(const CUSTDLG_INFO *custIn
> > >               TOOLBAR_Cust_AddButton(custInfo, hwnd, nIndexFrom,
> > > nIndexTo);
> > >           }
> > >       }
> > > - break;
> > >  case DL_CANCELDRAG:
> > >      /* Clear drag arrow */
> > >      DrawInsert(hwnd, hwndList, -1);
> >
> > Why? Looks to me it's fine to clear on dropped case too. Coverity is a
> > bit paranoid about missed breaks.
>
> A missing break is not trivial to detect; especially if something is
> done in a specific case. That's why Wine has a ton of "/* fall through
> */" annotations. IMHO they should be added here too .

Well, there's no third option basing on code - report all or report 
none, cause you can't figure out automatically is it ok to fall or not. 
A comment you mean is kind of suppression sign for checker?. Speaking 
about this particular case, I have a better comboex fix in mind, and for 
toolbar I think it's fine to add a comment.
> bye
>    michael

comctl32: Add missing break (Coverity)

[Marcus Meissner]

On Wed, Feb 23, 2011 at 08:24:46PM +0300, Nikolay Sivov wrote:
> > > >  }
> > > > - break;
> > > > case DL_CANCELDRAG:
> > > >     /* Clear drag arrow */
> > > >     DrawInsert(hwnd, hwndList, -1);
> > >
> > > Why? Looks to me it's fine to clear on dropped case too. Coverity is a
> > > bit paranoid about missed breaks.
> >
> > A missing break is not trivial to detect; especially if something is
> > done in a specific case. That's why Wine has a ton of "/* fall through
> > */" annotations. IMHO they should be added here too .
>
> Well, there's no third option basing on code - report all or report
> none, cause you can't figure out automatically is it ok to fall or
> not. A comment you mean is kind of suppression sign for checker?.
> Speaking about this particular case, I have a better comboex fix in
> mind, and for toolbar I think it's fine to add a comment.

lint used

/* FALLTHROUGH */

as a tag. Not sure if Coverity uses it, but we could use somesuch for
tagging such places.

Ciao, Marcus

comctl32: Add missing break (Coverity)

[Michael Stefaniuc]

Nikolay Sivov wrote:
> On 2/23/2011 19:56, Michael Stefaniuc wrote:
> > Nikolay Sivov wrote:
> > > On 2/23/2011 19:28, Amine Khaldi wrote:
> > > > CIDs 1581 and 1583.
> > > > @@ -1852,6 +1852,7 @@ COMBOEX_EditWndProc (HWND hwnd, UINT uMsg,
> > > > WPARAM wParam, LPARAM lParam,
> > > >       case VK_UP:
> > > >       step = -1;
> > > > -
> > > >    break;
> > > >     case VK_DOWN:
> > > >     /* by default, step is 1 */
> > > >     oldItem = SendMessageW (infoPtr->hwndSelf, CB_GETCURSEL,
> > > >
> > > > 0, 0);
> > >
> > > This is wrong.
> > > > @@ -2297,6 +2297,7 @@ static LRESULT
> > > > TOOLBAR_Cust_AvailDragListNotification(const CUSTDLG_INFO *custIn
> > > >               TOOLBAR_Cust_AddButton(custInfo, hwnd, nIndexFrom,
> > > > nIndexTo);
> > > >           }
> > > >       }
> > > > - break;
> > > >  case DL_CANCELDRAG:
> > > >      /* Clear drag arrow */
> > > >      DrawInsert(hwnd, hwndList, -1);
> > >
> > > Why? Looks to me it's fine to clear on dropped case too. Coverity is a
> > > bit paranoid about missed breaks.
> >
> > A missing break is not trivial to detect; especially if something is
> > done in a specific case. That's why Wine has a ton of "/* fall through
> > */" annotations. IMHO they should be added here too .
>
> Well, there's no third option basing on code - report all or report
> none, cause you can't figure out automatically is it ok to fall or not.
> A comment you mean is kind of suppression sign for checker?. Speaking

I don't really care about checkers but about people reading the code.
Adding code to silence a checker is bad taste; adding a comment to
improve the readability of the code is good.
> about this particular case, I have a better comboex fix in mind, and for
> toolbar I think it's fine to add a comment.

Cool, thanks.

bye
    michael