Hi Jan,
On 12/08/2025 08:32, Jan Beulich wrote:
On 11.08.2025 23:21, Julien Grall wrote:
On 11/08/2025 21:30, Dmytro Prokopchuk1 wrote:
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -330,9 +330,12 @@ shared_entry_header(struct grant_table *t, grant_ref_t ref)
/* Returned values should be independent of speculative execution */
block_speculation();
return &shared_entry_v2(t, ref).hdr;
+
+ default:
+ ASSERT_UNREACHABLE();
+ break;
}
- ASSERT_UNREACHABLE();
> block_speculation();>
return NULL;
I know you are trying to apply the MISRA rule. But this is odd that you
move the ASSERT_UNREACHABLE() but then code after is still only
reachable from the default. In fact, this is introducing a risk if
someone decides to add a new case but then forgot to return a value.
By moving the two other lines, the compiler should be able to throw an
error if you forgot a return.
I think we did discuss this pattern in the past. While moving everything up
to the "return" into the default: handling will please Eclair / Misra, we'll
then end up with no return statement at the end of a non-void function.
Beyond being good practice (imo) to have such a "main" return statement,
that's actually another rule, just one we apparently didn't accept (15.5).
Reading 15.5, this seems to be about having a single return in the
function. Unless I misunderstood something, this is different from what
you suggest.
Anyway, my main problem with this change is that ASSERT_UNREACHABLE() is
moved. I could possibly settle with:
default:
break;
}
ASSERT_UNREACHABLE();
...
But at least to me, this pattern is more difficult to read because I
have to look through the switch to understand the patch is only meant ot
be used by the "default" case.
Cheers,
--
Julien Grall
