From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast To: Liran Alon ,devel@edk2.groups.io From: "Sean" X-Originating-Location: Redmond, Washington, US (50.35.74.15) X-Originating-Platform: Windows Chrome 83 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Tue, 31 Mar 2020 15:54:27 -0700 References: <30a57406-0c09-043f-6b19-7e6e94afcb44@oracle.com> In-Reply-To: <30a57406-0c09-043f-6b19-7e6e94afcb44@oracle.com> Message-ID: <7506.1585695267608228111@groups.io> Content-Type: multipart/alternative; boundary="AvjcPaX7GtXqXTa4JKac" --AvjcPaX7GtXqXTa4JKac Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable I agree that safeintlib is not doing anything too interesting in this case = but that's not really the point.=C2=A0 The argument for it is that it becom= es the central point of code to check for safe conversions and an indicator= that the developer was thoughtful about this conversion and didn't just ca= st to avoid the compiler complaining.=C2=A0 If everyone starts putting thei= r own checks in place it leads to more code reviews, diversity in solutions= , and opportunities for bugs.=C2=A0 All that said those are soft reasons fo= r the change and that is up to you. @Laszlo - On the ASSERT part, I have a different view point and am more cu= rious about yours.=C2=A0 For release builds, I don't want to see CpuDeadLoo= ps anywhere unless I am ok with the device being returned/refunded.=C2=A0 O= ur error path would be to exit the function with an error code and potentia= lly log a ReportStatusCode.=C2=A0 =C2=A0I don't think you should continue i= n an invalid state as that just makes resolving the bug much much harder.= =C2=A0 =C2=A0 Given that the system can boot to at least a menu without th= is driver, it seems that failing out of the function would provide a better= "RELEASE" experience. Finally, given that this is contained in OVMF I am fine with whatever make= s the most sense for your platform and usecase. Thanks Sean --AvjcPaX7GtXqXTa4JKac Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable I agree that safeintlib is not doing anything too interesting in this case = but that's not really the point.  The argument for it is that it becom= es the central point of code to check for safe conversions and an indicator= that the developer was thoughtful about this conversion and didn't just ca= st to avoid the compiler complaining.  If everyone starts putting thei= r own checks in place it leads to more code reviews, diversity in solutions= , and opportunities for bugs.  All that said those are soft reasons fo= r the change and that is up to you.  

@Laszlo - On the= ASSERT part, I have a different view point and am more curious about yours= .  For release builds, I don't want to see CpuDeadLoops anywhere unles= s I am ok with the device being returned/refunded.  Our error path wou= ld be to exit the function with an error code and potentially log a ReportS= tatusCode.   I don't think you should continue in an invalid stat= e as that just makes resolving the bug much much harder.    Given= that the system can boot to at least a menu without this driver, it seems = that failing out of the function would provide a better "RELEASE" experienc= e. 

Finally, given that this is contained in OVMF I am fin= e with whatever makes the most sense for your platform and usecase.
Thanks
Sean --AvjcPaX7GtXqXTa4JKac--