On Thu, Oct 29, 2015 at 1:35 PM, Patrick Palka <patr...@parcs.ath.cx> wrote:
> On Thu, Oct 29, 2015 at 12:49 PM, David Malcolm <dmalc...@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 :)
