Hi Mike,

I earlier assumed that both the fallthrough were intentional and just made it evident by writing the comment. It was hence written actually for the code reader and not the static analyzer. The comment was similar in nature and style to existing No/1-byte/... data comments. Incidentally, the comment satisfied the static analyzer as well. It was beyond my expectation and I was pleasantly surprised too. Without digging deep into the particular code and to avoid any functional code change, I just thought that's it.

Your comment forced me to come back specifically to the particular code and to analyze it a bit more.

My analysis now says that the fallthrough doesn't serve any real functional purpose as there is absolutely no chance of if blocks of the fallthrough case(s) being executed later and not executed in the original case: and hence the fallthrough if it happens still leads to execution of return NULL; at the end of the function only. This is better communicated by adding break; statement in the original case: and in a way, one may agree with the static analyzer that there was a MISSING_BREAK. I should hence follow it up with a v2 patch for this.

Ranbir Singh

On Thu, Oct 5, 2023 at 12:17 AM Kinney, Michael D <michael.d.kinney@intel.com> wrote:

I do not prefer special comments for one static analyzer.

Is there an alternative design/implementation of this code
to make it more readable and not trigger any static analysis
false positives?

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
> Singh
> Sent: Tuesday, October 3, 2023 10:48 PM
> To: devel@edk2.groups.io; rsingh@ventanamicro.com
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Subject: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe:
> Fix MISSING_BREAK Coverity issues
>
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> The function GetNextHidItem has a switch-case code in which the
> case 1: falls through to case 2: and then case 2: falls through
> to case 3: in the block.
>
> While this may be intentional, it is not evident to any general
> code reader as well as any static analyzer tool. Just adding
>
> // No break; here as this is an intentional fallthrough.
>
> as comment in between makes a reader as well as Coverity happy.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
> MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> index acc19acd98e0..bc9a4824208b 100644
> --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> @@ -89,6 +89,10 @@ GetNextHidItem (
> return StartPos;
>
> }
>
>
>
> + //
>
> + // No break; here as this is an intentional fallthrough
>
> + //
>
> +
>
> case 2:
>
> //
>
> // 2-byte data
>
> @@ -99,6 +103,10 @@ GetNextHidItem (
> return StartPos;
>
> }
>
>
>
> + //
>
> + // No break; here as this is an intentional fallthrough
>
> + //
>
> +
>
> case 3:
>
> //
>
> // 4-byte data, adjust size
>
> --
> 2.34.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#109309):
> https://edk2.groups.io/g/devel/message/109309
> Mute This Topic: https://groups.io/mt/101750274/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>