Re: comctl32: Add missing break (Coverity)
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.
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
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
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
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
participants (3)
-
Marcus Meissner -
Michael Stefaniuc -
Nikolay Sivov