3 ms·

Author has a blog post here, https://ansel.photos/en/news/darktable-dans-le-mur-au-ralenti/ https://ansel.photos/en/news/darktable-dans-le-mur-au-ralent..., whi

by okl 3y ago

Author has a blog post here, https://ansel.photos/en/news/darktable-dans-le-mur-au-ralenti/ https://ansel.photos/en/news/darktable-dans-le-mur-au-ralent..., which exhibits some example code, for example, this: https://github.com/darktable-org/darktable/blob/darktable-4.2.x/src/gui/accelerators.c#L2898-L2918 https://github.com/darktable-org/darktable/blob/darktable-4.... That's a far cry from what I'd find acceptable in any project.

slibhb 3y ago
Excellent links. My favorite part: > However you put it, there is no valid reason for a software left open without touching it to turn the computer into a toaster, especially since we don’t buy Russian gas anymore.
nneonneo 3y ago
The author is not wrong, the code really is crazy. Here's another random snippet from the ugly rewrite: for(char relative[] = "-2"; (relative[0] ^= '+' ^ '-') == '-' || ++relative[1] <= '9'; ) gtk_list_store_insert_with_values(instances, NULL, -1, 0, relative, -1); This is just pure excess cleverness. It's not even obvious how many times this loop runs for. There's a comment 1000 lines away that says this: #define NUM_INSTANCES 5 // or 3, but change char relative[] = "-2" to "-1" This is just insanely unmaintainable code. If a string is warranted I would have just done the "stupid" thing with a `sprintf(buf, "%+d", relative);` to make it obviously correct, even if it seems "slower".
- sevagh 3y ago
  This just got added to some lists of interview questions.
- jiggawatts 3y ago
  I had to think about what that loop does for an embarrassingly long time. I’ve never seen XOR used as an in-place alternation operator like that in a for loop. I’ve only ever seen it used as a swap function not an inline expression. I’ve also never seen a for loop that uses a mutable string updated char-by-char like that. It’s just… special.
- zestyping 3y ago
  [flagged]

by okl 3y ago

slibhb 3y ago

Excellent links. My favorite part: > However you put it, there is no valid reason for a software left open without touching it to turn the computer into a toaster, especially since we don’t buy Russian gas anymore.

nneonneo 3y ago

The author is not wrong, the code really is crazy. Here's another random snippet from the ugly rewrite: for(char relative[] = "-2"; (relative[0] ^= '+' ^ '-') == '-' || ++relative[1] <= '9'; ) gtk_list_store_insert_with_values(instances, NULL, -1, 0, relative, -1); This is just pure excess cleverness. It's not even obvious how many times this loop runs for. There's a comment 1000 lines away that says this: #define NUM_INSTANCES 5 // or 3, but change char relative[] = "-2" to "-1" This is just insanely unmaintainable code. If a string is warranted I would have just done the "stupid" thing with a `sprintf(buf, "%+d", relative);` to make it obviously correct, even if it seems "slower".

sevagh 3y ago
This just got added to some lists of interview questions.
jiggawatts 3y ago
I had to think about what that loop does for an embarrassingly long time. I’ve never seen XOR used as an in-place alternation operator like that in a for loop. I’ve only ever seen it used as a swap function not an inline expression. I’ve also never seen a for loop that uses a mutable string updated char-by-char like that. It’s just… special.
zestyping 3y ago
[flagged]