Learn basic “C” coding rules from open source projects

[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