On 16.08.2023 12:47, Nicola Vetrini wrote: > On 16/08/2023 12:31, Jan Beulich wrote: >> On 16.08.2023 12:01, Nicola Vetrini wrote: >>> On 08/08/2023 11:03, Nicola Vetrini wrote: >>>> On 04/08/2023 08:42, Jan Beulich wrote: >>>>> On 04.08.2023 01:50, Stefano Stabellini wrote: >>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote: >>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote: >>>>>>>> Rule 2.1 states: "A project shall not contain unreachable code". >>>>>>>> >>>>>>>> The functions >>>>>>>> - machine_halt >>>>>>>> - maybe_reboot >>>>>>>> - machine_restart >>>>>>>> are not supposed to return, hence the following break statement >>>>>>>> is marked as intentionally unreachable with the >>>>>>>> ASSERT_UNREACHABLE() >>>>>>>> macro to justify the violation of the rule. >>>>>>> >>>>>>> During the discussion it was mentioned that this won't help with >>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to >>>>>>> effectively nothing. You want to clarify here how release builds >>>>>>> are to be taken care of, as those are what eventual certification >>>>>>> will be run against. >>>>>> >>>>>> Something along these lines: >>>>>> >>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to >>>>>> actually >>>>>> assert and detect errors, but it is also used as a marker to tag >>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't >>>>>> resolve >>>>>> into an assert, but retains its role of a code marker. >>>>>> >>>>>> Does it work? >>>>> >>>>> Well, it states what is happening, but I'm not convinced it >>>>> satisfies >>>>> rule 2.1. There's then still code there which isn't reachable, and >>>>> which a scanner will spot and report. >>>> >>>> It's not clear to me whether you dislike the patch itself or the >>>> commit >>>> message. If it's the latter, how about: >>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally >>>> unreachable code, which >>>> constitutes a motivated deviation from Rule 2.1. Additionally, in >>>> non-release >>>> builds, this macro performs a failing assertion to detect errors." >>> >>> Any feedback on this (with one edit: s/a failing assertion/an >>> assertion/) >> >> The patch here is kind of okay, but I'm afraid I view my earlier >> question >> as not addressed: How will the proposed change prevent the scanner from >> spotting issues here in release builds? Just stating in the description >> that there's a deviation is not going to help that. > > There is a deviation already in place. At the moment it just ignores > anything below an unreachable ASSERT_UNREACHABLE(), no matter what that > macro will expand to.
What do you mean by "in place"? docs/misra/ has nothing, afaics. And automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering things out of reports, aiui. (Plus of course there should be a single central place where all deviations are recorded, imo.) Jan
