This is the mail archive of the 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: Some real-life feedback on -Wmisleading-indentation

On Mon, 2016-01-11 at 15:20 +0800, Gerald Pfeifer wrote:
> Compiling Wine with GCC trunk (to become GCC 6) I noticed four
> dozen of warnings triggered by -Wmisleading-indentation.
> Some are simply weird formatting, some may be indicative of 
> real issues -- and I have started to look into them one by 
> one and submitting patches (to Wine).
> However, there is a pattern I noticed which, while a bit weird 
> in terms of formatting, does not really strike me as something
> -Wmisleading-indentation should warn about:
>     VariantInit(&ret);
>     hr = IWinHttpRequest_Invoke(request, ...);
>     ok(hr == DISP_E_UNKNOWNINTERFACE, "error %#x\n", hr);
>     VariantInit(&ret);
> if (0) /* crashes */
>     hr = IWinHttpRequest_Invoke(request, ...);
>     params.cArgs = 1;
>     hr = IWinHttpRequest_Invoke(request, ...);
>     ok(hr == DISP_E_TYPEMISMATCH, "error %#x\n", hr);
>     VariantInit(&arg[2]);
> This is from the Wine testsuite, and the if (0) in colum one guards 
> one invication of the function under test that would crash (so is 
> the equivalent of #if 0...#endif, except that it avoids conditional 
> compilation).
> Is this a bit unusual?  Definitely?  Does it look like a one of
> those cases a programmer would actually be tempted to misread?
> I don't think so.
> What do you think?

How often does this pattern occur in Wine?

In Chapter 31 of "Code Complete" (I'm looking at the 2nd edition),
McConnell discusses the use of whitespace in code layout; he talks about
both blank lines and indentation as being useful for providing hints to
the human reader about the structure of the code.

In several places he talks about the use of blank lines for separating
groups of related statements into "paragraphs", that blank lines in code
can and should be used to demarcate logical "blocks" of related
statements (e.g. pp747-8 of 2nd edition).

I think that in the Wine example above the blank lines do effectively
create "paragraphs" of code to a human, and I agree that a human reader
is unlikely to think of the

if (0)

as guarding the

   params.cArgs = 1;

since they're in different "paragraphs".

I apologize if I'm belaboring the point here, but I think that
-Wmisleading-indentation should make use of blank lines of code.

I've posted patches to do so here, firstly here:

which was rejected thusly by Jeff:
> I would argue that each of these does represent misleading
> indentation and that the warning is warranted for each.
> Perhaps they aren't as bad as prior cases, but I'd still
> consider them mis-leading.

and Bernd: 
> I think you could (barely) make a case for the last one being
> ok, but I disagree fairly strongly about the other two cases -
> they are clearly misindented, so why wouldn't we warn?

I later posted:
"[PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall"

which was rejected by Bernd:
> I personally see no use for the blank line heuristic, in fact
> I think it is misguided. To my eyes a warning is clearly
> warranted for the examples in this testcase. Do we actually
> have any users calling for this heuristic?

> That's a fairly low false-positive rate if we even want to
> call it that. I'd fix the three examples and leave the warning
> code as is. We can revisit the decision if we get a flood of
> bug reports.

and Pedro noted:
> IMHO, the problem with the levels idea will be if/when later
> you come up with some other orthogonal heuristic to catch some
> other class of indentation problem, and users want to enable
> it but not the blank-lines heuristic, or vice-versa.
> Also, levels don't allow finer-selection
> with "-Werror -Wno-error=misleading-indentation", IIUC.

In the light of Gerald's report on Wine, can we revisit this please, and
perhaps use the blank line heuristic for -Wall?

I'd like to keep the levels idea, and to simply have something this in
the docs:

"The warning can optionally accept a level (either 1 or 2), for
specifying increasing levels of strictness.
@option{-Wmisleading-indentation=1} is enabled by @option{-Wall} in C
and C++ and uses a set of heuristics intended to provide useful results
on typical code.
@option{-Wmisleading-indentation=2} is stricter."

or somesuch.

There may be an argument that -Wmisleading-indentation should be renamed
to -Wmisleading-whitespace, since indentation is a subset of whitespace.
I don't know how I feel about that.

Hope this is constructive

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