On 04/10/2023 11:52 am, Luca Fancellu wrote: > From the documentation: > > In the Xen codebase, these tags will be used to document and suppress > findings: > > - SAF-X-safe: This tag means that the next line of code contains a > finding, but > the non compliance to the checker is analysed and demonstrated to be > safe. > > I understand that Eclair is capable of suppressing also the line in which the > in-code suppression > comment resides, but these generic Xen in-code suppression comment are meant > to be used > by multiple static analysis tools and many of them suppress only the line > next to the comment > (Coverity, cppcheck). > > So I’m in favour of your approach below, clearly it depends on what the > maintainers feedback is: > >> /* SAF-2-safe */ >> if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && >> /* SAF-2-safe */ >> (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && >> /* SAF-2-safe */ >> (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) )
To be clear, this is illegible and a non-starter from a code maintenance point of view. It is bad enough needing annotations to start with, but the annotations *must* not interfere with the prior legibility. The form with comments on the end, that do not break up the tabulation of the code, is tolerable, providing the SAF turns into something meaningful. ~Andrew P.S. to be clear, I'm not saying that an ahead-of-line comments are unacceptable generally. Something like /* $FOO-$N-safe */ if ( blah ) might be fine in context, but that is a decision that needs to be made based on how the code reads with the comment in place.
