This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 10/29/2015 11:42 AM, Patrick Palka wrote:
On Thu, Oct 29, 2015 at 1:35 PM, Patrick Palka <patrick@parcs.ath.cx> 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 ();

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?)

However we should definitely not warn on

     if (foo)
       bar ();

       {
         baz ();
       }

Since that is valid GNU-style code :)
Wouldn't GNU style need the curleys lined up with the IF? and the call to baz() indented two spaces relative to the curleys?

That's the way I've always done things when I've wanted to introduce a binding scope like that.

jeff



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]