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


On 11/01/16 08:20, 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?

Gerald


Maybe I am missing something, but yes, this is misleading and confusing, and yes, it is weird indentation that I would normally assume is a mistake - and I would be very happy to see the compiler warn me about it.

Is there some particularly strong reason why the Wine code has this odd indentation?

I would say the best idea is to fix the oddity in the Wine code, not to try to weaken gcc's warnings as a workaround. The gcc warning is to spot unusual, misleading and possibly incorrect code - a warning on this code is correct. If gcc waters it down to workaround oddities in Wine, then it must surely also water it down for whatever oddities are found in other projects - until it becomes pointless.

So my vote (which is worthless, of course - I am a mere observer, but one who thinks "-Wmisleading-indentation" is a fantastic idea) is that the Wine developers most produce evidence that this code construction is necessary, clear, and important to their code, and also that the same technique is used in a wide variety of other projects, before it makes sense to reduce -Wmisleading-indentation to accommodate them. Even then I would make the variation require an additional flag or option to the -Wmisleading-indentation flag.



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