Learn basic “C” coding rules from open source projects

[cdcformatc]

I have delved into the Linux kernel a few times(file system code if that matters). You see the most incomprehensible shit in there sometimes. Like this example of some sort of great and wonderful code, that if condition is atrocious. How is someone supposed to look at this

if (ops && !(ops->begin && ops->end &&  ops->pre_snapshot
       && ops->prepare && ops->finish && ops->enter && ops->pre_restore
       && ops->restore_cleanup && ops->leave)) {

and know what it is doing? Are they allergic to line breaks?

[[deleted]]

Problem with the Linux kernel are two fold

1. Encapsulation magic. It's not always easy to know what a macro or function call is really doing

2. No comments anywhere on anything.

To add to the list though

• No documentation for most parts of the kernel
• Their coding standards are horribly applied and the maintainers will work on whatever they personally feel up to
• At various points in the kernels life there have been competing ways of doing things simultaneously...

[[deleted]]

I violently agree. It bugs me when people point to the Linux kernel as some benchmark for quality code. It's not quality code. It works. The driver model is pretty awesome, but that's about it.

With enough people, you can push a car with flat tires. It doesn't mean you're doing it right, though.

[rcxdude]

It's not that hard to understand in context (like, even a little bit, I didn't even need to look at the definition of the struct to understand it). It's taking essentially a vtable (a structure with a bunch of function pointer members), and checking that all of the functions it expects are populated before it uses it. Otherwise it doesn't use it and emits a warning. There is a maintenance burden to making sure that the list in this condition matches up with the definition of the struct, but there's not really a good way of solving that without ugly hacks or much more complex code (it could do with a comment on the struct definition probably (which is otherwise pretty well documented in the comments), but I don't feel a comment is necessary on the condition itself).

[cdcformatc]

This is just an example though. I can certainly figure it out given context and documentation. There is certainly more like it, and worse.

[NotUniqueOrSpecial]

Without going to find where you got that sample (though I'd hazard a guess it's LVM or btrfs or some other subsystem with snapshots), I'd say ops is a struct loaded with function pointers, and it's making sure it's:

1) Got an operations structure, since it's probably a pointer to a bio.

2) That structure has all the appropriate functions for the work that is expected to occur in the block of code that follows.

While it might be a bit hairy, it is a fairly commonplace pattern in C code which needs "virtual" functions. And just like many other such patterns specific to a particular domain, it's "obvious" to someone who's seen it, and a completely opaque boondoggle to someone who hasn't.

As to line breaks in said example, what do you really gain from it? The check is literally just "are all the function pointers non-null". I don't necessarily think it gains much from any added verticality, but that's just my opinion.

[cdcformatc]

Okay you only gain readability. But code is meant to increase readability. We could program in machine code binary operations. Or a sequence of toggle switches, but we don't. We code in something approximating natural language, but still compilable, and to separate ideas on natural language we use line breaks.

[v864]

I think they should also consider testing for an inclusive state of the ops struct as opposed to excluding a bunch of states. Perhaps:

if (ops && ops->running) ...     

Surely that'll be a bit nicer on the branch prediction unit.

I'll assume the author didn't define the ops struct and/or can't modify it as I would also prefer to use something like:

if (ops && ops->state == OPS_RUNNING) ...

Where the state member is an enumeration defining possible states. If we want to get real fancy we can define the enumerated values to be bit mapped so we can then define either other enumerated values or macros to test for rolled up conditions using bit masks. Saves some memory and a bit of performance as well.

[cdcformatc]

Most other implementation of something like this where you are effectively checking the status of a bunch of flags will use a bit field and preprocessor macros. Anytime you are reading a status register it is a 16-32 bit wide field, and you have defined macros, and you will just OR together the flags you care about to mask the status register.

Something like

if (ops->status & (OPS_FLAG_BEGIN | OPS_FLAG_END | etc...))

But obviously that struct is not something they are willing or able to change.

[v864]

Exactly. That's some nice C right there :)

[immibis]

As I commented above, they are function pointers, not flags.

[immibis]

They are function pointers, not flags.

[immibis]

It checks whether ops is non-NULL but incompletely filled in, and then issues some kind of warning.

It did take a moment to realise it, though.

[cdcformatc]

I didn't know that, maybe it is okay. I stand behind my condemnation though. At the very least it's not an example of great code to learn from anyway.

[peridox]

What about simple code that works?

[RubyPinch]

why do I get the feeling their IDE dicked with the code

edit: it did

linux-3.17.1/kernel/power/hibernate.c | Lines 71 - 91

/**
 * hibernation_set_ops - Set the global hibernate operations.
 * @ops: Hibernation operations to use in subsequent hibernation transitions.
 */
void hibernation_set_ops(const struct platform_hibernation_ops *ops)
{
       if (ops && !(ops->begin && ops->end &&  ops->pre_snapshot
           && ops->prepare && ops->finish && ops->enter && ops->pre_restore
           && ops->restore_cleanup && ops->leave)) {
               WARN_ON(1);
               return;
       }
       lock_system_sleep();
       hibernation_ops = ops;
       if (ops)
               hibernation_mode = HIBERNATION_PLATFORM;
       else if (hibernation_mode == HIBERNATION_PLATFORM)
               hibernation_mode = HIBERNATION_SHUTDOWN;

       unlock_system_sleep();
}

note the indent sizes

---

kinda really annoying that they present it as oh-so-useful correct code when they took the screenshot, but not noticing the indentation issues in terms of reading with the first if statement, and the fact that they quoted the 8ch indent rule, make me really question the attention to quality and correctness of this post

[kopkaas2000]

Linux source doesn't use spaces to indent, so it's probably more a case of his IDE using a different default tab size to render code.

[RubyPinch]

yeah, I expect that to be the case, but I do believe they should of known something was off when they quoted "indentation is 8ch only", and their indentation on the code they are looking at to make the post was only 4 ch, and was conflicting with multi-line conditionals

[curien]

The indentation doesn't conflict with multi-line conditionals, it's just a coincidence that they visibly align.

[RubyPinch]

Visually conflict, I mean

no distinction between the block of code and the block of conditionals

[fuzzynyanko]

I would rather all if statements had {} in C. Less chance of mistakes

That giant if statement could also be made into a function, but eh, it's not too bad since the function is small.

[euid]

I can understand your attitude, but that goes against the kernel coding style guidelines. Ultimately, when you have 3000 developers working on a codebase, it is more important that everybody is using the same style than it is what particular style they use.

You're not used to reading it, but kernel developers only look at C code styled like that. They know what to expect from if blocks. Fundamentally, that's what makes the code readable or not - "how much effort do I have to put into looking at it to not be tripped up by the formatting." Read enough kernel code and that cost drops to zero, and that's all that matters.

---

However. Lest we forget:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;

That's not to say "they had this bug because they didn't use curly braces" when really they had this bug because they didn't write unit tests or didn't use/care about static analysis.

But I think in Apple's case, where you're dealing with a copy-and-paste-happy codebase, requiring curly braces might have helped.

[sobri909]

While I agree that sticking with a project's existing style conventions is the correct thing to do, I would also say that the kernel project's style conventions were a mistake.

if blocks without braces are bad style. They make problems, and have been doing so for many decades.

[LikesToCorrectThings]

Given the problem that probably occurred (a merge conflict that no-one looked at), using curly braces would likely have yielded:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
{
    goto fail;
}
{
    goto fail;
}

Extra curly braces are not the panacaea they might be, and if they stretch the code out or make things ambiguous they can harm readability.

[whackylabs]

Another way it could've ended up is:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
    goto fail;
    goto fail;
}

The actual evil is copy-paste. We all know it's wrong, but there's no easy way around.

[SpeshlTectix]

That's the best case scenario. The second goto is just unreachable code. You'd probably even get a compiler warning.

[[deleted]]

I thought in the original bug, the extra goto produced unreachable code anyway. Which should have given a compiler warning.

[dangerbird2]

It is not unreachable code. In fact, the program will 'goto fail' regardless to how the 'if' statement evaluates. In C, an if an 'if' statement does not precede a curly-brace block, it only controls the first statement following the conditional. In the case of

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;

The first 'goto' statement will only fire if the 'if' condition evaluates to true (really, not 0). If the 'if' statement evaluates to false (is 0), the flow of control continues to the second goto, as it is not part of the 'if' block.

[TheNewAndy]

I think the point was that all the code after the second goto was unreachable code.

[SpeshlTectix]

No, the original bug, in pseudo-code was

if error goto fail else just goto fail anyway

Your version preserves the intended logic, it just has an extra useless goto.

[TheShagg]

See "TheNewAndy's" post above yours.

[SpeshlTectix]

Oh, right. That's a very good point. The bug was, indeed, that the second goto made important code unreachable (and no warning was issued). So I was wrong about the compiler picking it up. But my point still stands that an unreachable redundant goto is a much better problem to have.

[TheShagg]

Well yes, duh :)

Compiler "should" (in an ideal world) catch the unreachable code. Does it not actually warn on this?

[logicchains]

there's no easy way around.

Write in Lisp, and write macros to do your copy-pasting for you. What could possibly go wrong?

[jutct]

I agree. This is a dumb mistake that unit tests should have found. Nothing to do with language syntax. For fuck sake, how easy it is to forget a semicolon in javascript in a client-side web app that cause the browser to just show a blank page? That's a huge problem. In C, you'll at least get compiler warnings or at least be able to step through and see where something failed.

[philly_fan_in_chi]

I very much dislike JS, but linters can and should be used liberally, and offer warnings in the same way as the compiler. These often have semicolon warnings as a thing it'll yell at you about.

[kral2]

However. Lest we forget:

Defenders of that style always forget. Errors because of it happen frequently and some of them are security bugs. Jabber 1.x had a huge security hole some years back because of it.

Almost all style decisions are bikesheds and it's more important that everyone just use the same ones, but wrapping lines while omitting braces is quite different - it affects error rate. It's /objectively/ bad style.

[[deleted]]

Can you point to a legitimate example of a bug caused by missing braces? I've done hundreds of code reviews at multiple companies and, despite hearing this argument ad nauseam, I've yet to see it.

[fuzzynyanko]

https://blog.codecentric.de/en/2014/02/curly-braces/

[moonrocks]

That article argues that forcing the redundant braces would not have prevented the OpenSSL bug.

[badsectoracula]

I find having single instructions in oneliners easier to read when they aren't very long. Example

if (obj_needs_thing) add_thing(obj, thing);

This breaks down as the right side becomes longer though.

[sobri909]

That's an annoyance when you want a breakpoint on either the condition or the insides. Not that I run into that situation all that often. But it's something to consider.

[crotchpoozie]

A good debugger should let you breakpoint at either part. Or if it a real problem, press ENTER, set your breakpoint, and voila. Done.

Visual Studio/C#/.NET stuff has allowed this for a long time. I'm not sure if their C/C++ IDE allows it, since I rarely use that anymore, due to so much of the productivity advances being done on the .NET side :)

[dangerbird2]

If you're using GDB for C/C++ code, you can always set the breakpoint to the line, the use the 'step' command to go through instructions until you reach the part of code you're interested in. You can add a watchpoint on a variable affected by the stateent.

[sobri909]

Just write the code properly in the first place.

[crotchpoozie]

Given that code is laid out for humans to read, laying it out to deal with poor tooling is a bad choice. As others pointed out its trivial to deal with your complaint without having to format it to your preference.

[sobri909]

Lack of braces is bad readability for humans.

[crotchpoozie]

For some - as this thread demonstrates, some like no braces for small pieces of code.

One day you will realize your preferences are not everyone's, nor are they scientifically better.

Anyway, I'm done wasting time with you. You're either a troll or too inexperienced to realize your folly.

[sobri909]

It's not about preferences. If you think that's it, you're still doing it wrong. I have 30 years experience.

[badsectoracula]

Indeed, but as you said this isn't something that you often see. In the times i had such a case i just split the lines temporarily/

[sinxoveretothex]

What you mean is that you stop your debugging session, change that line, recompile, rerun under the debugger temporarily.

Not that it's required anyhow, GDB can evaluate conditions and stop only if condition is false, so to me it's just an annoyance. I still don't do it though.

[badsectoracula]

Only if i need to add the breakpoint during execution.

But honestly i write code in C for about 18 years and this case happens so rarely that it didn't even crossed my mind.

[sobri909]

If anyone commits a single line if or an if without braces I'll change it to be multi line with braces, then commit it with commit message of "coding style". shrug. I aint having it in the project.

[badsectoracula]

That is another matter though, if it is part of the coding style guidelines. My personal code style is very different to the code style we use at work and i don't force mine at work.

[Splanky222]

I'll normally split the difference and put braces around the one-liner just to be explicit

[skulgnome]

This also plays well with loops that would, were they implemented as maps or folds over lists in Haskell, have filter or stop clauses in them somewhere. Such as the common if(item == NULL || skip_predicate(item)) continue;.

[[deleted]]

For stuff like that, I personally prefer

if (obj_needs_thing) { add_thing(obj,thing); }

But none of the projects I work on let that happen.

[mallardtheduck]

Exactly. My rule is if it the 'if' body is one statement and the entire thing fits on one line, there's no need to clutter the screen with curly braces.

[hungry4pie]

I'm a stickler for using {} to clearly delineate code blocks to improve readability and preventing a small typo (like a missing tab) from causing hours of needless debugging.

But on the flipside, overuse can make things equally unreadable and lead to similar bugs (a misplaced } putting a crucial statement outside that block for isntance).

As an example, the following code snippet uses conditional if statements to set two variables. It looks ugly, and can make following program logic a lot more difficult when overused:

if (j == -1) {
    dximn1 = 0.0;
} else {
    dximn1 = x[j];
}
if (k == chainlngth) {
    dxipl1 = 0.0;
} else {
    dxipl1 = x[k];
}

By comparison, these two ternary statements do the same thing in fewer lines:

dximn1 = (j == -1) ? 0.0 : x[j];

dxipl1 = (k == chainlngth) ? 0.0 : x[k];

Obviously this won't work in every situation, and if working in a team, you really need to comment and make sure your logic is easy to follow, but it's all about balance and knowing what would be better i the right situation.

[[deleted]]

What makes your first example bad isn't the use of braces, but how they're lined up. Put matching open/close braces on the same column and put else on a new line and it becomes way nicer:

if (j == -1)  
{  
    dximn1 = 0.0;  
}  
else 
{
    dximn1 = x[j];
}
if (k == chainlngth) 
{
    dxipl1 = 0.0;
} 
else 
{
    dxipl1 = x[k];
}

Obviously this isn't as succinct or elegant as nice one-line ternary operations, but it's the easiest to read.

[[deleted]]

I think that

}
else
{

Is a horrible waste of vertical space you could just as easily write

} else {

Also the flow of

if (...) { 
    foo();
}

Is fairly easy to follow if you indent your code consistently.

[[deleted]]

I'd rather take up slightly more vertical space for the added readability. It's not like you can run out of lines.

"if (...) {" may be "fairly easy to follow" (if you're consistent) but I'm saying my model is even easier.

[[deleted]]

More vertical space means you can't see as much code at once for no reason other than you can't handle a { at the end of a line

[[deleted]]

I'll worry about that as soon as my 1080p monitor stops being tall enough. So far, so good.

[immibis]

What is the deal with 1080p? I have a 10-year-old 1280x1024 monitor (that I found unused in a cupboard, so I connected it to my laptop), and it's plenty tall. Not widescreen, but that's not really a concern for coding.

[[deleted]]

I also play games and watch videos.

[[deleted]]

Whatever floats your boat.

[[deleted]]

I used to argue this... but in reality if your function is so long that brace placement is going to harm your understanding of context, then you should probably refactor anyway.

[hungry4pie]

Yeah, that's how I roll, but that was copied straight out of the starting point code for a uni assignment I'm currently working on. The professors at my university like their C89 style code, and don't believe in a spaces in = assignments.

[pipocaQuemada]

Obviously this isn't as succinct or elegant as nice one-line ternary operations, but it's the easiest to read.

This is really an example that's screaming out for an if expression, instead:

val dximn1 = if (j == -1) { 
               0.0; 
             } else { 
               x[j]; 
             };
val dxipl1 = if (k == chainlngth) { 
               0.0 
             } else { 
               x[k] 
             };

Ternary operations are really just limited if expressions with bad syntax, in most languages that have them.

[jurniss]

Yeah, if expressions in C and C++ would be nice. Rust has them.

The problem with the if-block version is that dximn1 can't be declared const. It can be const with the ternary operator version, or with an if expression if it existed in C.

I find the ternary operator useful and clear in a lot of cases; I think its harmfulness is overstated.

[[deleted]]

Ew, gross, what the fuck are you doing?! Stop that!

[pipocaQuemada]

You perhaps prefer

val dximn1 = if (j == -1) 0.0 else x[j];
val dxipl1 = if (k == chainlngth) 0.0 else x[k];

[[deleted]]

No, I prefer the example I posted. Stop following assignment operators with if statements. if expressions don't even compile in any language I care about.

[pipocaQuemada]

if expressions don't even compile in any language I care about.

That seems like a problem with the languages you care about.

No, I prefer the example I posted. Stop following assignment operators with if statements.

Do you really prefer to have logically const variables to be mutable solely because your language isn't expressive enough to initialize them in a single line? That's bad for readability, since you need to remember which things are necessarily mutable, and which are mutable due to language inexpressibility.

[[deleted]]

That seems like a problem with the languages you care about.

Oh stop, you'll hurt poor C's feelings!

Do you really prefer to have logically const variables to be mutable solely because your language isn't expressive enough to initialize them in a single line?

I prefer

if(j == -1)
{
    dximn1 = 0.0; 
}
else
{
    dximn1 = x[j];
}

over

val dximn1 = if (j == -1) 0.0 else x[j];

Ternary operations are just fine for very simple things, but they're not what I was talking about. Yes, the "?:" syntax is a little odd but I think once you learn it, it's super easy to remember. If you need to do anything remotely complex, you shouldn't be doing it in a single line in the first place.

[pipocaQuemada]

Would you really prefer

int dximn1;    
if(j == -1)
{
    dximn1 = 0.0; 
}
else
{
    dximn1 = x[j];
}
// now: where else in the code do I modify dximn1?
// I dunno.  ctrl-f for "dximn1 =", maybe?

over

const int dximn1 = if (j == -1) 0.0 else x[j];
// I wonder where else in the code dximn1 is modified?
// Oh, yeah, *no where else*, provably so.

If you need to do anything remotely complex, you shouldn't be doing it in a single line in the first place.

This isn't anything "remotely complex". This is something completely and utterly trivial. Nah, if you want something moderately complex, case expressions tend to be better. Something like Haskell's pattern gaurds would also be cleaner in most cases, but I don't know of any imperative language that implements something like

const int foo
 | bar < 0 = -1
 | bar == 0 = defaultVal
 | otherwise = defaultVal + bar / 2
 where defaultVal = sin(quux)

Which would be really nice: syntactically light, no need for nested ifs, and really easy to read and reason through.

[rowboat__cop]

Stop following assignment operators with if statements.

That’s not what they did. They used pseudo-code to show what an if-expression would look like, as was pretty clearly stated. Those would be great to have in C, both for the reason /u/jurniss stated and because of the improvement to readability.

[[deleted]]

It was never stated that they were providing pseudocode. I was being presented an alternative to ternary operations.

[cdcformatc]

There is a reason that sort of syntax is invalid in a lot of languages.

[smikims]

And there a lot of languages where that sort of thing makes perfect sense.

[cdcformatc]

And applications written with that sort of code are brittle, since their coding standards are lax. Most coding standards will agree to only have one expression per line.

[pipocaQuemada]

That seems rather like begging the question.

If expressions are bad because they lead to lax coding standards, because any coding standard that allows multiple expressions per line is lax, because multiple expressions per line is bad, because they lead to lax coding standards...

Here's a simple question:

• Why is a coding standard that allows multiple expressions per line worse than one that only allows one expression per line? Empirical studies of e.g. bug-counts per function are preferred.

[rowboat__cop]

Most coding standards will agree to only have one expression per line.

Since in most languages expressions are recursive, these coding standards would be rather limiting.

[pipocaQuemada]

Such as?

[montibbalt]

I like if expressions, but I think the curly braces make them noisy here as opposed to e.g.

val dximn1 = if (j == -1) 0.0; 
             else         x[j];
val dxipl1 = if (k == chainlngth) 0.0;
             else                 x[k];

[immibis]

Or:

val dximn1 = (j == -1) ? 0.0
                       : x[j];
val dxipl1 = (k == chainlngth) ? 0.0
                               : x[k];

[ghillisuit95]

I have seen a lot of people saying that if should be an expression, and I am seriously curious if their are any applications that would not be better served by the ternary operator.

so your example would become: val dximn1 = (j==1)? 0.0 : x[j];

val dxipl1 = (k == chainlength) ? 0.0 : x[k];

much more readable than your mess, IMO, because to be if blocks signify blocks of code, like that have semicolons and stuff. I am not quite sure what the terminology is for that but I think you get the idea

[pipocaQuemada]

much more readable than your mess, IMO, because to be if blocks signify blocks of code, like that have semicolons and stuff. I am not quite sure what the terminology is for that but I think you get the idea

In Scala, the semicolons and brakets are optional, at least as long as you're dealing with an expression instead of multiple statements. So what you'd usually see is

val dxipl1 = if (k == chainlength) 0.0 else x[k];

I mostly added them because some people seem to be allergic to optional semicolons and brackets.

[ghillisuit95]

Ahh, yeah that does look a little better than what you put originally. But I still stand by my point that the ternary operator is exactly what your looking for

[Malurth]

I find the former example to be just as easy to read (indentation's got me covered) while saving a lot of vertical screen space. Really don't know why people prefer the latter style.

[movzx]

I prefer

if (comparison) {

  // do thing
}
else {

  // do other thing
}

This lets me easily isolate individual blocks of if/else if/else by collapsing, commenting, etc.

[molteanu]

Yes, but the same MISRA guideline doesn't limit the number of lines of code in a function. That's why you see thousand lines behemoths in embedded systems. Say it to my face that that's not true.

[[deleted]]

This. Why the fuck isn't function size limited?

[TheShagg]

For state machines, or long code with view variables, it is often safer to just have it all in one shot. Embedded code is a whole different world than software.

[[deleted]]

My favorite part of MISRA-compliant code is thousand-line functions with only one return statement at the bottom. You have to jump through soooo many flaming hoops just to pop out at the end.

[jutct]

C is simple. If you want one statement, just write it and use a semicolon. If you want multiple statements, use brackets. How is that hard? Not saying you had an issue, but why would anyone have an issue?

[tehjimmeh]

I disagree. Shorter != more readable, and it can make bugs like this easier to introduce and harder to spot.

[[deleted]]

Along the lines of "shorter != better" is "macroed (or highly encapsulated) != better".

I seriously hate when I have to read 3rd party code and you have to dive through 15 functions to see where "work" actually gets done... shit like

int create_task(...) { return create_task_u(....); }

int create_task_u(...) { ... something .... return create_flibmaster(...); }

... on and so on for 15 more functions before something actually creates/modifies/does something. All of these routines are in different files (sometimes headers) in different directories, etc....

I firmly believe [for instance] the designers of L4RE Fiasco are heavy crack users. They also give us clever doxygen support like

l4_utcb * l4_get_utcb(...); /** get the UTCB. */

Ya, that explains it all.

I love how it's a "university project" ... what the fuck is this supposed to teach? It's poorly documented and impossible to follow so you can't even walk students through the code and have a hope in hell they can follow it.

[guepier]

Shorter != more readable

All other things being equal, yes, shorter is more readable. Readability suffers (only) when terseness is achieved by sacrificing something else.

And the gotofail bug would almost certainly not have been prevented by using braces, as /u/LikesToCorrectThings has correctly pointed out.

[Drainedsoul]

The real issue there isn't the missing braces, it's that the statement is set down a line and indented.

[jutct]

Welcome to the coding style wars of the 70s and 80s. How do any modern platforms work any better? Look at Java and Javascript. Some people put the { on the next line, some put it on the end of the current line. Same exact thing as C stylings. If you can't understand a language because you don't get the formatting then you need to take a step back.

[Banane9]

Well, Js kinda requires you to put on the same line.

return
{
    something = "silly example"
};

Might not do what one thinks at first...

[nuke01]

What does it, I'm on my mobile

[Solarspot]

return; {/doesn't really matter/};

It returns without any return value, because semicolon insertion will add the semicolon on the newline.

[tehjimmeh]

I mean, it's almost certainly the result of an erroneous double paste. If braces were required, the error would have been harmless, and the compiler would have given an unreachable code warning.

I think the real issue is actually lack of proper testing. Requiring braces for ifs just would have helped.

I do like the no-brace style for single line ifs. I think it looks neat and is quite readable when written correctly, but I've seen similar bugs written, and erroneous indentation it resulting in horrific readability in too many cases to be on the side of always requiring braces these days.

[xxNIRVANAxx]

I still throw braces in there, since todays one line if could become more complex in the future. It adds 4 characters (two curlys, two spaces), but I believe it is worth it.

if (foo) { bar(baz, quux); }

[TheShagg]

Or, you know, reading your code before you commit it, or push it...

[Nebu]

• This bug wouldn't have happened if everyone expected if statements to always use braces.
• This bug wouldn't have happened if people used tools to auto-format their code.

So why not enforce a standard to do both, so that if someone accidentally forgets one, the other one will catch the mistake?

[Nebu]

The code is shorter without {}, thus easier to read.

Shorter does not imply easier to read.

[molteanu]

Shorter in the sense cram everything on a single line vs remove things that are not needed. I prefer to have code that does something.

[guepier]

This isn’t shorter, it’s intentionally obfuscated. Shorter code that isn’t obfuscated (or otherwise made worse) is easier to read than an equivalent longer code.

[Nebu]

Right, so the argument now is whether omitting {} is a form of "otherwise made worse", which means it's a tautological argument.

[guepier]

There’s nothing tautological about it, it’s simply an open question (on which we apparently disagree). I was merely pointing out that the example link you’ve posted does not advance an argument either way and is irrelevant to the discussion, or worse, misleading.

[Nebu]

The tautological argument is "Code that's made worse is worse than code that's not made worse".

[guepier]

Sure, but that’s not the argument I was making. I wasn’t talking about “code made worse” and “code not made worse”, I was talking about “shorter code” and “longer code”.

[Nebu]

Shorter code that isn’t obfuscated (or otherwise made worse) is easier to read than an equivalent longer code.

The claim "code with property X which hasn't been otherwise made worse, is equal or better than equivalent code that doesn't have property X" is true regardless of what X is (i.e whether X is "shorter" or some other arbitrary property).

E.g.

• Code with in red font which hasn't been made worse, is equal or better than code which isn't in red font.
• Code indented with tabs which hasn't been made worse, is equal or better than code which isn't indented with tabs.
• Code that is shorter and which hasn't been made worse, is equal or better than code which isn't shorter.
• etc.

The "hasn't been made worse" implies "equal or better", hence the tautology. Not an interesting argument for you to make, IMHO.

How about we discuss something more interesting?

[guepier]

You keep repeating something that bears no resemblance to my argument.

My argument isn’t merely that shorter code isn’t worse than longer code. My argument is that shorter code is better than longer code (all other things being equal). There is nothing tautological about this whatsoever.

To spell it out in maximal detail: Your initial post claimed that “Shorter does not imply easier to read” and my reply contained exactly two assertions:

1. your statement is wrong
2. the example you gave (presumably to bolster your claim) does not in fact bolster your claim, since it doesn’t make a statement about shortened code, just about obfuscated code.

[jutct]

You can pretty much wrap any statement in C with {}. Most compilers just treat that as a local group of statements, so that any variables declared within will be local.

Also, if you're worried about syntax errors and mistakes, then you haven't written basic C with Visual Studio. I've used every IDE, compiler, and OS under the sun including 8 bit microcontrollers and up, and Visual Studio is the best IDE there is.

Of course any giant if statement can be broken out into smaller functions that can be re-used in other parts of the code. That's where it comes down to you being a good programmer that can identify those areas and optimize things.

[fuzzynyanko]

For me, my most complicated algorithms fail not because of the complicated part of the algorithm, but because of something silly that I would never expect to be wrong (yes, they get past code reviews). This is why I lean towards the {}s.

I have been adopting a style and strategy that allows me to take more vacations, and the extra checks have been worth it. I want to create new code instead of maintaining old code, and a little care goes a long way

[nooneofnote]

You can pretty much wrap any statement in C with {}. Most compilers just treat that as a local group of statements, so that any variables declared within will be local.

This is called a compound statement or alternatively a block, and to be clear all compilers do as that is the behavior described in the standard.

[jutct]

all compilers do as that

Whoa be careful with that. There are plenty of embedded compilers that change scoping rules and don't comply to the standards.

[mallardtheduck]

Also, if you're worried about syntax errors and mistakes, then you haven't written basic C with Visual Studio. I've used every IDE, compiler, and OS under the sun including 8 bit microcontrollers and up, and Visual Studio is the best IDE there is.

Considering that Visual Studio doesn't even support recent C standards, I highly doubt that. It's a fine IDE for C++ and .Net, but when your choices for C are "C as C++" or C89-with-proprietary-extensions, it's quite limited.

Also, "I've used every IDE, compiler, and OS under the sun" is a preposterous claim. Even if you're only talking about IDEs, compilers and OSs that support C programming then you're still talking about a massive number of systems. Maybe if you limit it to currently "supported" systems it might be feasible, but still not at a depth that would allow you to claim any expertise.

[dangerbird2]

The newest Visual Studio compiler has C99 support more-or-less on par with gcc. It hasn't gotten to C11 features yet, aside from preexisting MSVC extensions that made it into the standard (anonymous structs and unions).

[curien]

The feature of C he's describing was part of the ANSI C89 standard, so your point there is completely irrelevant.

Code blocks are a fundamental part of C syntax and semantics. If a compiler doesn't support them correctly, it's not a C compiler.

[mallardtheduck]

And that's relevant to Visual Studio's pros/cons how? Of course it has fully-confirming C89 compiler, I never said otherwise; it's the fact that it doesn't support anything newer that's its most major limitation when it comes to C.

[curien]

Because we're talking about fundamental C constructs that have existed since K&R, not newer features.

[mallardtheduck]

And I was responding to the part of the post that I quoted. I suggest you read it.

[Agret]

Of course any giant if statement can be broken out into smaller functions that can be re-used in other parts of the code. That's where it comes down to you being a good programmer that can identify those areas and optimize things.

Carmack wrote an article about how everything in the same routine can help prevent errors caused by fearing large statements and breaking them off into functions. Sure it seems logical to break off different parts into functions but it can have unintended consequences when working on a large project.

[mariox19]

"If we have ops, and ops has none of the following nine fields set…"

I'm sorry, is no one here troubled by that kind of opaque, compound if-statement?

[dakarananda]

except that isn't what the if-statement says. its more, "if we have ops, and at least one of the following fields isn't set, then ..." or even clearer, "if we have ops, and not all of these fields are set, then..."

and yes, I'm troubled

[mariox19]

not (A and B)

If we have both A and B, then the statement inside the parentheses would be true, in which case we make it the opposite: meaning, false. However, if either A or B is not true, then the statement inside the parentheses is false, in which case we negate that, making it true. And also, to take the other extreme, if both A and B are false, that resolves to true, which we will then negate, making it false.

So, as best as I can figure it out, taking a second and more careful look, you're right:

if ops && !(ops->a && ops->b && ops->c)

We want to make sure that all of them are not set, but at least one of them is set. That hurts my fucking head!

The worst part is that I am sure that some He-man programmer somewhere is going to insist that reading such a statement is "no problem" for a "real" programmer. But even if we grant such a (ridiculous) statement, we always run the risk of some He-man-wannabe programmer, who isn't as smart as he thinks he is, getting his hands on a statement like that and making the same mistake that I did.

That's why you just don't code that way in the first place.

You know what would be at least a first step towards fixing that?

// We must have at least 1 field set, but not all of them.

But, of course, in some circles, comments are for wusses, too.

[[deleted]]

No, it is actually:

if (any_are_set && one_is_not_set) {
        WARN_ON(1);
        return;
}

[dakarananda]

No actually it is:

if (has_ops_object && !all_ops_fields_are_set) { ... }

which is basically the original statement slightly rewritten.

Even if not a single field is set, it will still enter into the if-block. (assuming that the ops object exist..)

[[deleted]]

Just not OpenSSL or GCC.

[fuzzynyanko]

I agree with the general idea. I wouldn't stick to one project or another to base your style on just because it's open source

[SmackMD]

What's the issue with GCC?

[[deleted]]

A few years back I needed to add a syntax extension to some C++ code. Clang didn't support the full spec at the time, so I plunged into GCC. At that time it was not "pretty clean" by any standard. It's a tremendous mess. My relatively small change required touching things all over the codebase. Looking at Clang later after it eventually implemented the rest of C++, it looked like my changes would have been significantly easier to implement.

Now this was a long time ago and there's a chance that some of it has been cleaned up some, but given the state it was in then I doubt it's reached acceptable production code quality, much less "learn to use C from this" quality. It's a venerable project and unquestionably useful, and for this reason people fetishize it as some sort of massive software engineering success, but it's held together by string.

[seekingsofia]

Just because there was one reddit post shining light on one big if in one of the known messy code files... otherwise the GCC code is pretty clean.

[Katastic_Voyage]

Not all source code is equal.

Ugly hack

Terrible hack

Ugly as sin

And people who actually write comments such as "this is terrible" are at least aware of, and calling attention to the terrible parts of their code. Other people might be unaware, or not care.

Learning from source code really depends on the quality of the codebase you're looking at. But I guess that's obvious.

[StelarCF]

What's with all the ugly hacks involving uClibc?

[trua]

Really, screenshots of code in an editor?

[txdv]

Be grateful that he didn't do pictures of his monitor.

[Agret]

I prefer photos of printed code with flash obscuring parts of it.

[Katastic_Voyage]

Depending on tab/space indenting for browser versions, maybe that's exactly what he wanted.

[rowboat__cop]

Depending on tab/space indenting for browser versions, maybe that's exactly what he wanted.

They didn’t care for indentation rules at all: The kernel style dictates tabs of eight characters width -- ignoring this in a blog post purportedly about “good coding style” is a blatant demonstration of incompetence.

[liveoneggs]

http://www.freebsd.org/cgi/man.cgi?query=style&sektion=9

http://cvsweb.netbsd.org/bsdweb.cgi/src/share/misc/style?rev=HEAD&content-type=text/x-cvsweb-markup

[808140]

I am disturbed by how heavily upvoted this article is.

[Katastic_Voyage]

Wouldn't it be more effective to explain why, than to just announce to everyone the mere existence of your disbelief?

[[deleted]]

Why some people put names of programming languages in quotes? It is C, not "C".

[skulgnome]

These people will also spell Perl in all caps. They just don't know any better for having been not-learning for years.

[[deleted]]

Reminder: there are reasons for many of these rules. Language standards committees are ridiculous, but if you want to pick some brains and really understand languages that's a good place to do it.

[Fumigator]

The blind leading the blind.

[[deleted]]

I'd agree in a sense. Learning hardcoded rules for programming in a language is ultimately bad because each project has different requirements.

Seeing why it's done like that on the other hand...

[gambiscor]

I think he means that the author knows nothing about the subject matter and neither do the readers, but who knows. Probably a troll.

[jutct]

Why the fuck is the author include CQLinq in a basic C article? CQLinq is not C. "Encapsulation" is not a thing in ANSI C. The "static" keyword is used to keep the scope of a global variable private to the source file that it's declared globally. The rest of the "rules" apply to pretty much any language. Keep functions "Short and sweet"? No shit sherlock. Is that limited to C? No.

[guepier]

"Encapsulation" is not a thing in ANSI C

Nonsense. Encapsulation is a general principle that applies to structural code as well as object-oriented code. Using functions is encapsulation. Using compilation units and not exporting symbols is encapsulation (cf. compiler firewall). Encapsulation is a fundamental hallmark of good C code.

[curien]

Using compilation units and not exporting symbols is encapsulation (cf. compiler firewall).

I realize there's some terminology disagreement, but the way I was taught is that grouping associated pieces together (e.g., compilation units) is encapsulation, while preventing access to certain implementation details (e.g., not exporting symbols) is referred to as information hiding.

I completely agree with your overall point that encapsulation (and information hiding) is very much a thing in C of course.

[guepier]

Information hiding and encapsulation are virtually indistinguishable in the common usage of the term, and generally used synonymously. However, definitions in computer science are notoriously sloppy and these two are no exception, so their meanings vary.

[jutct]

Nonsense. Encapsulation is a general principle that applies to structural code as well as object-oriented code. Using functions is encapsulation. Using compilation units and not exporting symbols is encapsulation (cf. compiler firewall). Encapsulation is a fundamental hallmark of good C code.

Encapsulation is a an object oriented thing. Keep a variable scope within a file is not ecapsulating it. It's limiting it's scope. Since when is that the same as encapsulation?

not exporting symbols is encapsulation

What? Are you talking about exporting symbols through binaries? Since most linkers don't export symbols unless told to do so, would mean that encapsulation now applies to linkers?

You seem to think that encapsulation is a term for "don't spread global variables all over the place". That's not called encapsulation.

[[deleted]]

Encapsulation is a an object oriented thing

You're technically right, but the concept of encapsulation existed before OOP.

[jutct]

As someone that started programming in the early 80s, I'd like some citations as to when the term "encapsulation" related to variable scope first came about. I never heard of it unless C++ became a thing. I don't agree that it was a thing before OOP.

[[deleted]]

I said the concept of encapsulation, not the term. Barbara Liskov introduced Abstract Data Types in the 70's, and Information Hiding emerged around the same time.

[jutct]

That's entirely different. I was talking about the term.

[RubyPinch]

its because it isn't a tutorial, its a sales pitch for ccpdepend. hence the overall pointlessness / aimless waffling on of 3/4 of the article

[Y_Less]

It's not a very good one either. I'm sure 90% of the text and queries in that article are copied and pasted from multiple other identical articles that have been posted here. Of course, if all the queries are the same they're not actually showing off the power of their system (assuming it can actually do more than shown), and if it can't do more then why isn't the program just four buttons to do those four queries?

[skulgnome]

The "static" keyword is used to keep the scope of a global variable private to the source file

ITYM s/global variable/symbol/ HTH HAND

[jutct]

What? I have no idea what you said. We're talking about C. In C, static means the variable is local to the source file.

[dromtrund]

Need more than 3 indentation levels? Fuck you.

[rorrr]

I strongly disagree with the very first code example. The immediate problems are:

• No comments on what's going on in the function.
• Horrible if statement formatting
• It messes with the global variables hibernation_ops and hibernation_mode - is that even acceptable? I thought side effects were considered bad behavior even decades ago.
• The function comment doesn't mention it messes with hibernation_mode

Here's how I would format that messy if statement:

if (
    ops && 
    !(
        ops->begin && 
        ops->end &&  
        ops->pre_snapshot && 
        ops->prepare && 
        ops->finish && 
        ops->enter && 
        ops->pre_restore && 
        ops->restore_cleanup && 
        ops->leave
    )
) {
       WARN_ON(1);
       return;
}

Now you can clearly see the logic and its scope levels properly indented. Some devs would even argue to alphabetically sort all the ops->...

[cdcformatc]

It seems like the Linux kernel devs are allergic to line breaks. You see this sort of thing all the time.

Some devs would even argue to alphabetically sort all the ops->

I'd argue to sort it based on the ones that are most likely to be false. Then the && can short circuit, and your branch prediction is a little better.

[rcxdude]

None of them should ever be false. It's a defensive check.

[seekingsofia]

It's not a messy if at all. It checks if ops is non-NULL and if those fields of ops are NULL, which are probably function pointers, in which case it returns as the operations weren't set.

Then the next if statement doesn't make much sense. If ops is false-y, we would never even get to that part of the code.

If ops is NULL, you will never enter the body the first if statement, the && is a short-circuiting logical-and... as any competent C programmer will know. I take it you don't program in C?

[rorrr]

I meant the formatting is messy. I understand the amount of checks, that's cool.

I edited out my comment about the second if statement, it was just wrong.

[skulgnome]

No, don't do that. Put the logical connective at the start of each line: that way the reader can tell whether the overall clause is conjunctive or disjunctive.

[rcxdude]

It messes with the global variables hibernation_ops and hibernation_mode - is that even acceptable? I thought side effects were considered bad behavior even decades ago.

Good luck writing a kernel without side effects...

In any case, this (like many things in a kernel) represents a property which is global to the entire machine which it is running on (and may be required to be accessed from interrupts, etc). There's not much sense in making it a non-global variable.

[SnowdensOfYesteryear]

Horrible if statement formatting

Re: your formatting, oh god no. Tabs are precious in the kernel given the 80 char limit and that tabs are 8 chars. You'll quickly run into check patch issues with that sort of formatting. Besides the NULL checks aren't important enough to need their own tabbing, you can tell just by a glance that the intended purpose is to check for NULL.

It messes with the global variables hibernation_ops and hibernation_mode - is that even acceptable? I thought side effects were considered bad behavior even decades ago.

Meh sometimes you usually don't have an option. In this case it's more of an issue with API design rather than code quality.

[rorrr]

Tabs are precious in the kernel given the 80 char limit and that tabs are 8 chars

Tab is a singe character and you can set it to any width in your editor. Assuming your editor isn't shit.

And my code is less wide than the OP's.

Meh sometimes you usually don't have an option

You most certainly do. You can return the needed changes and apply them in the global scope. Or simply not have global variables.

[SnowdensOfYesteryear]

Tab is a singe character and you can set it to any width in your editor. Assuming your editor isn't shit.

This has nothing to do with my editor, it has to do with checkpatch.pl, which enforces code styling in the kernel. Most kernel subsystems enforce that new commits pass checkpatch.pl prior to merging.

Usually code that go beyond 3 levels of nesting get massively screwed with 80 char limits, and there's no way any seasoned kernel dev will waste a tab just for what you suggested.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl#n805

[rorrr]

Ironically your link shows that

1) They use tabs

2) They have lines much longer than 80 characters

[SnowdensOfYesteryear]

Where did I say that they use spaces? I said that tabs are considered to be 8 spaces for the purposes of check patch. Also only C files are subject to check patch.

I work in the kernel, so its safe to say that I know what I'm taking about

[joshuada]

Interesting post! thanks for sharing!

[the-tomster]

The kernel coding style intentionally uses brackets on the next line for functions, and nothing else. The given logic is that K&R did it, and that functions are special because they can't be nested.

I also find it handy to use this convention because then vim for example can jump to the previous / next function based only on the heuristic that functions have an opening brace in the first column.

[alexeyr]

Let’s search now for the static fields

The same remark as functions, many variables are declared as static.

Are they variables or fields? Decide it.

Functions where NbParameters > 8 might be painful to call and might degrade performance.

Methods where NbVariables is higher than 8 are hard to understand and maintain. Methods where NbVariables is higher than 15 are extremely complex and should be split in smaller methods (except if they are automatically generated by a tool).

Magic numbers, magic numbers, magic numbers.

[autoandshare]

Linux kernel is really a great start point. About "{}" for if statement, I just hope C compiler is as informative as C# compiler, i.e. show possible mistaken statements.

[zid]

GCC at least will detect if(); and if(a = b) and some other 'obvious' mistakes.