On Mon, 06 Mar 2023, Jiri Slaby wrote:
> On 06. 03. 23, 10:04, Lee Jones wrote:
> > On Mon, 06 Mar 2023, Jiri Slaby wrote:
> >
> > > On 03. 03. 23, 15:11, Lee Jones wrote:
> > > > On Wed, 22 Feb 2023, Florian Eckert wrote:
> > > > > @@ -113,21 +207,38 @@ static void ledtrig_tty_work(struct work_struct *work)
> > > > > trigger_data->tty = tty;
> > > > > }
> > > > > - ret = tty_get_icount(trigger_data->tty, &icount);
> > > > > - if (ret) {
> > > > > - dev_info(trigger_data->tty->dev, "Failed to get icount, stopped polling\n");
> > > > > - mutex_unlock(&trigger_data->mutex);
> > > > > - return;
> > > > > - }
> > > > > -
> > > > > - if (icount.rx != trigger_data->rx ||
> > > > > - icount.tx != trigger_data->tx) {
> > > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
> > > > > -
> > > > > - trigger_data->rx = icount.rx;
> > > > > - trigger_data->tx = icount.tx;
> > > > > - } else {
> > > > > - led_set_brightness_sync(trigger_data->led_cdev, LED_OFF);
> > > > > + switch (trigger_data->mode) {
> > > > > + case TTY_LED_CTS:
> > > > > + ledtrig_tty_flags(trigger_data, TIOCM_CTS);
> > > > > + break;
> > > > > + case TTY_LED_DSR:
> > > > > + ledtrig_tty_flags(trigger_data, TIOCM_DSR);
> > > > > + break;
> > > > > + case TTY_LED_CAR:
> > > > > + ledtrig_tty_flags(trigger_data, TIOCM_CAR);
> > > > > + break;
> > > > > + case TTY_LED_RNG:
> > > > > + ledtrig_tty_flags(trigger_data, TIOCM_RNG);
> > > > > + break;
> > > > > + case TTY_LED_CNT:
> > > >
> > > > I believe this requires a 'fall-through' statement.
> > >
> > > I don't think this is the case. Isn't fallthrough required only in cases
> > > when there is at least one statement, i.e. a block?
> >
> > There's no mention of this caveat in the document.
> >
> > To my untrained eyes, the rule looks fairly explicit, starting with "All".
> >
> > "
> > All switch/case blocks must end in one of:
> >
> > * break;
> > * fallthrough;
> > * continue;
> > * goto <label>;
> > * return [expression];
> > "
> >
> > If you're aware of something I'm not, please consider updating the doc.
>
> The magic word in the above is "block", IMO. A block is defined for me as a
> list of declarations and/or statements. Which is not the case in the above
> (i.e. in sequential "case"s).
>
> Furthermore, the gcc docs specifically say about fallthrough attribute:
> It can only be used in a switch statement (the compiler will issue an error
> otherwise), after a preceding statement and before a logically succeeding
> case label, or user-defined label.
>
> While "case X:" is technically a (label) statement, I don't think they were
> thinking of it as such here due to following "succeeding case label" in the
> text.
>
> So checking with the code, gcc indeed skips those
> (should_warn_for_implicit_fallthrough()):
> /* Skip all immediately following labels. */
> while (!gsi_end_p (gsi)
> && (gimple_code (gsi_stmt (gsi)) == GIMPLE_LABEL
> || gimple_code (gsi_stmt (gsi)) == GIMPLE_PREDICT))
> gsi_next_nondebug (&gsi);
>
>
> Apart from that, fallthrough only makes the code harder to read:
>
> case X:
> case Y:
> case Z:
> default:
> do_something();
>
> VS
>
> case X:
> fallthrough;
> case Y:
> fallthrough;
> case Z:
> fallthrough;
> default:
> do_something();
>
> The first one is a clear win, IMO, and it's pretty clear that it falls
> through on purpose. And even for compiler -- it shall not produce a warning
> in that case.
Works for me. Thanks for the clear explanation, Jiri and Uwe.
And yes Uwe, it would be good if we could make that clearer in the doc.
--
Lee Jones [李琼斯]
