On 05/10/2023 8:43 am, Luca Fancellu wrote: >> On 5 Oct 2023, at 00:46, Stefano Stabellini <[email protected]> wrote: >> >> Hi MISRA C working group (Jan, Roger, Andrew, Julien, Bertrand, George) >> >> in a recent thread Andrew pointed out that the SAF-2-safe tag is >> confusing and requested a rename: >> https://marc.info/?l=xen-devel&m=169634970821202 >> >> As documented by docs/misra/documenting-violations.rst: >> >> - 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. >> - SAF-X-false-positive-<tool>: This tag means that the next line of code >> contains a finding, but the finding is a bug of the tool. >> >> >> Today we have already 28 instances of SAF tags in the Xen codebase. >> >> >> Andrew suggested "ANALYSIS" instead of SAF so I would imagine: >> - ANALYSIS-X-safe >> - ANALYSIS-X-false-positive-<tool> >> >> If we really want a rename I suggest to rename SAF to SAFE: >> - SAFE-X-safe >> - SAFE-X-false-positive-<tool> >> >> Or maybe MISRA: >> - MISRA-X-safe >> - MISRA-X-false-positive-<tool> >> >> But I actually prefer to keep the tag as it is today. > We chose a generic name instead of MISRA because the tag can potentially > suppress findings > of any checker, including MISRA checker. > > If SAF-* is confusing, what about FUSA-* ? > > Anyway I’m thinking that every name we could come up will be confusing at > first, improving the > documentation would mitigate it (by improving I mean to improve the fruition > of it, for example a > Read the docs documentation has the search bar, a quick copy paste of SAF- > would make the > documenting-violations page visible.)
No - this is a problem *because* changing the documentation doesn't help. (To be clear, updating the documentation is fine, but irrelevant here.) These are annotations in code. They need to be: 1) Short (obviously) 2) Clear to someone who isn't you (the collective us of this group) reading the code. 3) Non-intrusive, so as not to get in the way of the code. and they must be all three. This was even a principle given at the start of the MISRA work that we would not be deteriorating the quality of the code just to comply. Point 3 is other thread about end-of-line, or block regions. Lets leave that there because it's really a metadata transformation problem constrained by where the comments can acceptably go. Point 2 is the issue here, and "SAF-$N-safe" scores very highly on the WTF-o-meter *even* for people who know that it's something related to MISRA. Seriously it looks like someone couldn't spell, and everyone else went with it (reflects poorly on everyone else). And yes, I know it's an initialisation for something, but it's not even an industry standard term - it's a contraction of an intentionally generic phrase, with substantial irony on an early MISRA call where there was uncertainly between people as to what it even stood for. These are the thoughts running through the minds of people reading the code when they don't understand what they're looking at. Annotations for other static analysers intentionally use their own name so they're googleable. Guess what SAF googles for? Sustainable Aviation Fuel, or Specialist Automotive Finance. Fine, lets be more specific. How about "Xen SAF" ? Nope... "Did you mean to search for: Xen SAVE Xen SAN Xen VIF Xenstaff" Despite many of the search results referencing patches, or rendered documents out of docs/, not a single one of them gets documenting-violations.rst in any form, where the single definition of this term is hiding in a paragraph which spends 90% of it's volume describing a monotonically increasing number. Seriously, ChatGPT would struggle to make shit this good up. The thing we tag with *must* be trivially recognisable as an analysis tag in order for others to be able to read the code. Therefore, it needs to be an actual full world (hence the ANALYSIS suggestion), or an industry standard term (where MISRA does qualify). I don't exactly what it is - something else might turn out to be even better, but it is very important to be not this, for everyone else to have an easy time reading the code. And reasoning along that line... What's wrong with just /* octal-ok */ or /* womble-permitted */ so it's also apparent in context what the contentious issue might be and why it might be mitigated? The mechanics behind the scenes is just a trivial text replacement, and the tagging scheme does not have to uniform obfuscated identifier for that to work. ~Andrew
