Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[David Malcolm <dmalcolm@redhat.com>]

On Thu, 2015-10-29 at 13:35 -0400, Patrick Palka wrote:
> On Thu, Oct 29, 2015 at 12:49 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> > Attempting to bootstrap gcc with -Wmisleading-indentation enabled I ran
> > into a few failures where the indentation, although bad, was arguably
> > not misleading.
> >
> > In regrename.c:scan_rtx_address:
> >
> >   1308      case PRE_MODIFY:
> >   1309        /* If the target doesn't claim to handle autoinc, this must be
> >   1310           something special, like a stack push.  Kill this chain.  */
> >   1311      if (!AUTO_INC_DEC)
> >   1312        action = mark_all_read;
> >   1313
> >   1314        break;
> >               ^ this is indented at the same level as the "action =" code,
> >               but clearly isn't guarded by the if () at line 1311.
> >
> > In gcc/fortran/io.c:gfc_match_open:
> >
> >   1997          {
> >   1998            static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL };
> >   1999
> >   2000          if (!is_char_type ("DELIM", open->delim))
> >   2001            goto cleanup;
> >   2002
> >   2003            if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
> >   2004                                            open->delim->value.character.string,
> >   2005                                            "OPEN", warn))
> >                   ^ this is indented with the "goto cleanup;" due to
> >                     lines 2000-2001 not being indented enough, but
> >                     line 2003 clearly isn't guarded by the
> >                     "if (!is_char_type" conditional.
> >
> > In gcc/function.c:locate_and_pad_parm:
> >
> >   4118        locate->slot_offset.constant = -initial_offset_ptr->constant;
> >   4119        if (initial_offset_ptr->var)
> >   4120          locate->slot_offset.var = size_binop (MINUS_EXPR, ssize_int (0),
> >   4121                                                initial_offset_ptr->var);
> >   4122
> >   4123          {
> >   4124            tree s2 = sizetree;
> >   4125            if (where_pad != none
> >   4126                && (!tree_fits_uhwi_p (sizetree)
> >   4127                    || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
> >   4128              s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
> >   4129            SUB_PARM_SIZE (locate->slot_offset, s2);
> >   4130          }
> >                 ^ this block is not guarded by the
> >                   "if (initial_offset_ptr->var)"
> >                   and the whitespace line (4122) is likely to make a
> >                   human reader of the code read it that way also.
> >
> > In each case, a blank line separated the guarded code from followup code
> > that wasn't guarded, and to my eyes, the blank line makes the meaning of
> > the badly-indented code sufficiently clear that it seems unjustified to
> > issue a -Wmisleading-indentation warning.
> 
> This makes sense to me.
> 
> Though I've been thinking about proposing a simpler and more relaxed heuristic:
> 
>     if (next_stmt_exploc.line - body_exploc.line > 1)
>       return false;
> 
> That is, don't warn if there are any lines between the (start of the)
> body statement and the next statement.
> 
> This would catch the presence of blank lines as well as code like:
> 
>     if (foo)
>       bar (an_argument_1,
>            an_argument_2);
>       baz ();

If I'm following you, I think I disagree: my (subjective) opinion is
that we ought to warn for such a case.  I ran into an example of this on
the linux kernel:
https://bugzilla.kernel.org/show_bug.cgi?id=98261

In linux-4.0.3/drivers/scsi/bfa/bfa_ioc.c: bfa_cb_sfp_state_query:

  3673              if (sfp->state_query_cbfn)
  3674                      sfp->state_query_cbfn(sfp->state_query_cbarg,
  3675                                            sfp->status);
  3676                      sfp->portspeed = BFA_PORT_SPEED_UNKNOWN;

Line 3676 is indented as if guarded by line 3673's "if", when it's not.
I would want us to emit a warning about this, despite the presence of
line 3675 (body_exploc would be on line 3674, next_stmt_exploc on line
3676).


> and
> 
>     if (foo)
>       bar ();
>       /* Some comment.  */
>       baz ();
> 
> Though I am not confident that we should not warn in such cases. At
> this point whether some code is misleadingly indented or not becomes
> pretty subjective (so it may be better to not warn?)

My (subjective) opinion is that we also ought to warn for this case.

Clearly, there's something of an "eye of the beholder" effect here.  My
thinking with the blank lines approach is that if a human reader is
quickly scanning over the code, an entirely blank line is much more
noticeable than code or comments: it breaks up the apparent "structure"
of the code more.  I don't have hard data to back this up though.

(My feelings on indentation and the "look" of code may be being
influenced by my Python background - I worked on Python before working
on GCC)


> > The attached patch suppresses the warning for such a case.
> >
> > OK for trunk?
> >
> > gcc/c-family/ChangeLog:
> >         * c-indentation.c (line_is_blank_p): New function.
> >         (separated_by_blank_lines_p): New function.
> >         (should_warn_for_misleading_indentation): Don't warn if the
> >         guarded and unguarded code are separated by one or more entirely
> >         blank lines.
> >
> > gcc/testsuite/ChangeLog:
> >         * c-c++-common/Wmisleading-indentation.c (fn_40): New function.
> > ---
> >  gcc/c-family/c-indentation.c                       | 58 ++++++++++++++++++++++
> >  .../c-c++-common/Wmisleading-indentation.c         | 32 ++++++++++++
> >  2 files changed, 90 insertions(+)
> >
> > diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
> > index 5b119f7..d9d4380 100644
> > --- a/gcc/c-family/c-indentation.c
> > +++ b/gcc/c-family/c-indentation.c
> > @@ -77,6 +77,42 @@ get_visual_column (expanded_location exploc,
> >    return true;
> >  }
> >
> > +/* Determine if the given source line of length LINE_LEN is entirely blank,
> > +   or contains one or more non-whitespace characters.  */
> > +
> > +static bool
> > +line_is_blank_p (const char *line, int line_len)
> > +{
> > +  for (int i = 0; i < line_len; i++)
> > +    if (!ISSPACE (line[i]))
> > +      return false;
> > +
> > +  return true;
> > +}
> > +
> > +/* Helper function for should_warn_for_misleading_indentation.
> > +   Determines if there are one or more blank lines between the
> > +   given source lines.  */
> > +
> > +static bool
> > +separated_by_blank_lines_p (const char *file,
> > +                           int start_line, int end_line)
> > +{
> > +  gcc_assert (start_line < end_line);
> > +  for (int line_num = start_line; line_num < end_line; line_num++)
> 
> Shouldn't this loop begin at start_line + 1?

Good catch; thanks.