9 ms·
GCC 6 Will Warn You About Misleading Code Indentations
- jimrandomh 11y ago> "Specifically, a warning is issued for if, else, while, and for clauses with a guarded statement that does not use braces, followed by an unguarded statement with the same indentation." > "The warning is not issued for code involving multiline preprocessor logic ... The warning is not issued after a @code{#line} directive, since this typically indicates autogenerated code" Seems like a good thing to have, and pretty conservative about not causing false positives. In particular, constructs like double-iteration should be fine: for(y=0; y<ymax; y++) for(x=0; x<xmax; x++) { //... } Which I checked because I like this construct and people very rarely think about it.
- noobie 11y agoI apologize for the stupidity of my question but shouldn't that be for(y=0; y<ymax; y++) { for(x=0; x<xmax; x++) { //... } } ? Edit: Oh, I get it, {} are only "mandatory" if we have more than one instruction, the second for loop gets counted as one instruction and hence the omitting of {}. Thanks!
- avar 11y agoNo, the two mean the same thing in C, e.g.: if (x) y; Is the same as: if (x) { y; }
- Macha 11y agoJust in case anyone who has never used C (or Java, or C# etc etc) is confused, it's not the indentation that is causing this meaning here: if (x) y; also has the same meaning.
- recursive 11y agoWhy would it be? Certainly not for readability. I think it was posted as intended.
- Sir_Cmpwn 11y agoIt works without the additional braces. Consider this: for (i=0;i<imax;i++) printf("%d\n", i); You can omit the braces if the body of the loop is a single block, just like an if statement. You can also do this: if (a == b) if (b == c) { do_thing(); } Same deal with the for loop. The nested loop is a single block.
- deleted 11y ago[deleted]
- halosghost 11y agoIn C, control flow like if, for, while, do, else, etc., take the rest of the line of code (i.e., till the next semi-colon) and runs it under the conditions of the clause. braces—{}—allow you to take multiple lines of code and have them be guarded at the same time rather than having to repeat the condition. GP is taking advantage of the fact that the next for clause will be considered a single statement, and therefore the second set of braces are not technically needed. However, omitting braces for single-line snippets has caused plenty of havoc in the past (cf. Apple's gotofail). Some would say it is better to always include braces in control flow.
- jimrandomh 11y agoIf I were doing it that way, I wouldn't give the outer blocks' curly braces their own lines. For example in C#, where pretty much every file has a single "namespace" block containing a single "class" block, I write it as: namespace SomeNamespace { class SomeClass { //... }} Which avoids having shifted the whole file an extra indent level to the right. But I haven't seen anyone else doing this.
- Avshalom 11y agoI'm not writing C# but yeah: AS3 and Java both have the Package{class{ two-step before you actually write any code. I don't even indent inside class: package{ class{ main{ now we indent } }}
- threeseed 11y agoYou don't indent after a package e.g. https://github.com/apache/spark/blob/master/core/src/main/java/org/apache/spark/JavaSparkListener.java https://github.com/apache/spark/blob/master/core/src/main/ja... And it's fair enough to indent with a main since it is so infrequent. Ideally you only have one main entry point.
- JoshTriplett 11y agoThat pattern still seems like obfuscation to me. Why not indent the inner for loop, to make the control flow more obvious? For instance, consider how "break" or "continue" inside the braces would work.
- contravariant 11y agoI don't see how 'continue' would be confusing, it would stop the current calculation and start the next one, same as with normal loops. The behaviour of 'break' might be confusing but then that's always the case in nested loops, no matter how you notate them.
- wasyl 11y agoI would argue that `break` intuitively makes you go back an indent, so if you don't indent after the first `for`, one can assume `break` just exits both loops (goes back from lvl 1 to lvl 0 indent)
- jimrandomh 11y agoThis doesn't quite work, since break skips over if statements (and most breaks have at least one if statement around them). So you have to look glance at the keyword at the elbow of each indent anyways, to know where it's going.
- jimrandomh 11y agoThe reason for doing it that way, is to emphasize when two loops together are conceptually iterating over a single thing, and have some symmetry, like iterating over the cells in a 2D grid. It makes it possible to line up the elements of the two loops, emphasizing the similar structure. I agree that it would be very bad form to use "break" inside a block like that. But in the cases where this construct applies, it would still be bad form to break if the outer block had curly braces and indentation, because skipping the rest of a row isn't a natural operation in the sort of way that skipping the rest of a collection is.
- chris_wot 11y agoEw! That's not any better at all... Man's gotta have an opinion. At least C doesn't have optional semi-colons.
- cnvogel 11y agoWhile I understand that it's mostly a question of aesthetics, I don't like this also because it might break the expected semantics of continue/break in such a loop. When I iterate over things more complex than a single variable where the conditions in a for() would become ugly, I tend to put the domain-specific "increment" into a small static function: static int next_pixel(*x, *y, width, height) { (*x)++; ... if (last pixel...) return 0; return 1; } /* later on... */ do { /* do something with coordinates x, y */ } while (next_pixel(&x, &y, WIDTH, HEIGHT)); This is, of course, not only restricted to 2D arrays, but could cover walking of a tree or a filesystem...
- Spl3en 11y agoWhat about this ? for(y=0; y<ymax; y++) { for(x=0; x<xmax; x++) { //... } }
- eru 11y agoI'd put the two loop-headers on the same line in that case.
- thiht 11y agoIf I see this, I'll immediately think it's a bug, the difference between x and y is not even obvious at first sight, it looks like the line was duplicated accidentally.
- userbinator 11y agoThis would certainly be a useful feature, especially for those learning C, but I wonder whether checking stylistic things should really be in the compiler or part of a separate tool.
- chris_wot 11y agoThe genie is out of the bottle already, gcc quite sensibly will warn you if it sees an assignment operator in an if statement. I'm sure it's saved countless man hours and pretty awful bugs since it was introduced :-)
- ifdefdebug 11y agoI hope it doesn't warn for things like if(0 != (i = getValue()))
- KindDragon 11y agoPVS Studio support similar warning V640 http://www.viva64.com/en/d/0258/ http://www.viva64.com/en/d/0258/
- leni536 11y agoNo more "goto fail;"?
- rwmj 11y agoI wonder if it'll be able to make sense of our macro-abusing XML-in-C: https://github.com/libguestfs/libguestfs/blob/master/src/launch-libvirt.c#L1054 https://github.com/libguestfs/libguestfs/blob/master/src/lau... (Scroll up a couple of pages to see the definitions of the macros)
- jsizz 11y agoAnd thus, every boneheaded project that ships with -Werror enabled will inflict an excrement tornado of FAIL on its grateful users and packagers. THIS IS WHY YOU SHOULD NOT USE -Werror. NOT EVER. (And if you can't bring yourself to do anything about mere warnings, you shouldn't be in software engineering.)
- tomjakubowski 11y agoFrom the article: > This new warning isn't enabled by default. And even if it were, it's a matter of adding -Wno-misleading-indentation to make builds green again. Not a big deal. And besides, it's likely that fixing those warnings could be done mechanically. (and it's hardly true that every project that ships with -Werror has such misleading indentation somewhere in its code)
- cperciva 11y agoAnd even if it were, it's a matter of adding -Wno-misleading-indentation to make builds green again How does that help the person who is building the release you shipped six months ago with a new compiler?
- legulere 11y agoYou should have -Werror enabled on debug builds when developing and disabled on release builds.
- MereInterest 11y agoHuh. I would think it to be the other way around. When debugging, let me comment stuff out without erroring due to an unused variable or an unused parameter. Let me use a C-style cast to check the bit representation of an object. Then when building the release build, enable -Werror to make sure that I'm not sweeping anything under the rug.
- ArkyBeagle 11y agoSeems like simply using astyle would be an improvement over catching this at compile-time.
- nn3 11y agoThe point of the warning is to find bugs, not enforce a particular coding style. Running astyle would make it worse because you would hide the bug.
- ArkyBeagle 11y agoHaving used astyle for exactly this, I simply have to disagree. Of course, you have to look at the code between running astyle and compiling it.
- jongraehl 11y agoclang-format is better
- __david__ 11y agoNice, maybe this will finally quiet the wrongheaded opinion that braces should always be used.
- yoz-y 11y agoI do not think that the opinion of mandatory braces is wrong - in a language that does have braces. If not for anything else, then for the fact that yi{ will _always_ yank the current block for me. Omitting braces will ruin this.
- hoodoof 11y agoSo whitespace matters huh?
- seanwilson 11y agoI wish more languages would have what are essentially compiler enforced style guidelines (e.g. indentation, variable/function naming, file naming). So much time is wasted debating style, dealing with code review comments about style and making/using related tools.
- yoz-y 11y agoI agree, although I think it should be done by a tool separate from the compiler. Purely because I'd have a stable compiler first and more code = more bugs.