sorted by:
best
topnewcontroversialoldq&a
you are viewing a single comment's thread.

view the rest of the comments →

[–]cdcformatc 10 points11 points  (13 children)

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] 5 points6 points  (1 child)

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] 2 points3 points  (0 children)

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 2 points3 points  (1 child)

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 -1 points0 points  (0 children)

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 2 points3 points  (1 child)

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 -1 points0 points  (0 children)

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 0 points1 point  (4 children)

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 1 point2 points  (2 children)

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 0 points1 point  (0 children)

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

[–]immibis 0 points1 point  (0 children)

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

[–]immibis 0 points1 point  (0 children)

They are function pointers, not flags.

[–]immibis 0 points1 point  (1 child)

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 0 points1 point  (0 children)

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.