Coding Horrors: You can gather all your technical debt into one easy framework!

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1503 »

Edison was a dick: Apr 3, 2010; direct current only

rjmccall posted:

Clearly you're complaining about GNU whitespace conventions.

I often do, but this isn't GNU coding style, GNU coding style is worse.

This code has the opening brace on the same line for blocks.
GNU style prefers

code:

if (condition)
  {
    stuff
  }

I can see the rationale for it, since a compound statement is still a statement, and the first statement after a control structure is indented, but it's an incredible waste of whitespace!

# ? Apr 27, 2013 11:10

Adbot: ADBOT LOVES YOU

# ? May 17, 2024 20:17

Jabor: Jul 16, 2010; #1 Loser at SpaceChem

Doctor w-rw-rw- posted:

You do realize that "do{ ... } while(false);" is, in other places, a practical and useful construct in C, right? It creates a block around which multiple expressions or statements are still treated as one so as to not disrupt braceless if's.

Sure, if you're writing a macro. Not so much anywhere else.

# ? Apr 27, 2013 11:13

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

Doctor w-rw-rw- posted:

You do realize that "do{ ... } while(false);" is, in other places, a practical and useful construct in C, right? It creates a block around which multiple expressions or statements are still treated as one so as to not disrupt braceless if's.

The horror was *supposed* to be that this guy was amazingly getting around the "single entry, single exit, no goto" rules by using a do...while (FALSE);, but:

rjmccall posted:

That is a rather awesome mistake; I'm not sure I've seen it before. It takes talent to soldier on against that kind of misparenthesization.

If the clang static analyzer doesn't catch it, you should file a bug.

Holy gently caress :aaaaa:

. I didn't even see that.

# ? Apr 27, 2013 13:45

Movac: Oct 31, 2012

"Do while false" isn't a coding horror, it's a low-budget and high performance exception handling mechanism used when you find the use of goto a slippery slope. :rolleyes:

VVV To be clear, I didn't know about it either until I went and looked for more examples. Never expected to see it advocated.

Movac fucked around with this message at 14:49 on Apr 27, 2013

# ? Apr 27, 2013 14:00

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

I didn't even know it was "a thing". To be honest, we all use goto to do fast exit and cleanups.

# ? Apr 27, 2013 14:21

baquerd: Jul 2, 2007; by FactsAreUseless

Extracting the block out into a separate function would be preferable, especially if you're unit testing.

# ? Apr 27, 2013 14:25

Jabor: Jul 16, 2010; #1 Loser at SpaceChem

If you can use scope-based cleanup instead that's better (either RAII, or using directives, or even just try-finally), but if you can't then goto is really the best option.

Goto has always been the low-level option you turn to if your language doesn't natively support a particular control-flow pattern you want to implement.

# ? Apr 27, 2013 14:26

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

baquerd posted:

Extracting the block out into a separate function would be preferable, especially if you're unit testing.

Show me how you would do that on the code I wrote then.

Jabor posted:

If you can use scope-based cleanup instead that's better (either RAII, or using directives, or even just try-finally), but if you can't then goto is really the best option.

Goto has always been the low-level option you turn to if your language doesn't natively support a particular control-flow pattern you want to implement.

If MSVC (and let's be honest here, SunPro) supported gcc cleanup attributes or an RAII equivalent in C, I'd switch over in heartbeat. It's a nice feature.

# ? Apr 27, 2013 14:31

Plorkyeran: Mar 22, 2007; To Escape The Shackles Of The Old Forums, We Must Reject The Tribal Negativity He Endorsed

Suspicious Dish posted:

Show me how you would do that on the code I wrote then.

C++ code:

image_t *img_load_from_filename(char *filename) {
    FILE *f;
    image_t *ret = NULL;

    char *path = path_join(DATADIR, filename);
    if (!path)
        return NULL;

    f = fopen(path, "r");
    if (f == NULL)
        return NULL;

    ret = read_image(f);

    fclose(f);

    return ret;
}

image_t *read_image(FILE *f) {
    image_header_t img_header;

    if (!img_read_header(&img_header, f))
        return NULL;

    ret = malloc(sizeof(image_t) + 8 * img_header.data_size);
    if (ret == NULL)
        return NULL;

    if (img_read(ret, f))
        return ret;

    free(ret);
    return NULL;
}

# ? Apr 27, 2013 15:04

Volmarias: Dec 31, 2002; EMAIL... THE INTERNET... SEARCH ENGINES...

Suspicious Dish posted:

The horror was *supposed* to be that this guy was amazingly getting around the "single entry, single exit, no goto" rules by using a do...while (FALSE);, but:

Holy gently caress . I didn't even see that.

Came to complain about do/while, shocked when goons found out that his code was horribly broken. The coding horror here is you, Suspicious Dish.

# ? Apr 27, 2013 16:00

Edison was a dick: Apr 3, 2010; direct current only

Suspicious Dish posted:

If MSVC (and let's be honest here, SunPro) supported gcc cleanup attributes or an RAII equivalent in C, I'd switch over in heartbeat. It's a nice feature.

Yep, hidden behind appropriate macros it makes for nicer code, the systemd guys use it roughly like this

code:

#define _cleanup_(x) __attribute__((cleanup(x)))

static inline void freep(void *p) {
        free(*(void**) p);
}
#define _cleanup_free_ _cleanup_(freep)

int somefunc(void) {
    _cleanup_free_ int *array = NULL;

    if (somecondition) {
        array = calloc(20, sizeof(int));
    }

    return 0;
}

# ? Apr 27, 2013 16:58

floWenoL: Oct 23, 2002

Edit: Ugh, never mind, the coding horror is me.

floWenoL fucked around with this message at 18:40 on Apr 27, 2013

# ? Apr 27, 2013 18:37

QuarkJets: Sep 8, 2008

Sedro posted:

code:
ret = malloc (sizeof (image_t)) + 8 * img_header.data_size;
Edit: but the coding horror is do ... while (false)

Wow, there are some layers, here. At first I thought that if you changed while(false) to while(success) that it would fix the code, but then I realized that it just creates an infinite loop when success becomes TRUE. Then I realized that the programmer intentionally did this to make the code flow in a specific way; he wanted the code chunk in the do/while loop to run exactly once, but he also wanted to be able to break out of it and then run some other cleanup code if anything went wrong. There are better ways to do this, but breaking the do/while loop does give the desired flow. Neat

e: Or what everyone above me said, that'll teach me to read further before posting. This is the kind of thing that you do when you want a single point of return and aren't comfortable with passing pointers, I guess.

QuarkJets fucked around with this message at 19:32 on Apr 27, 2013

# ? Apr 27, 2013 19:28

Foxfire_: Nov 8, 2010

The thing also leaves the file open if some errors happen and opens the file in (probably) the wrong mode.

The sensible thing is just to use gotos for the control flow instead of making a funnylooking goto out of do...while and break:

code:

image_t* img_load_from_filename(const char* filename)
{
    FILE* f = NULL;
    boolean success = FALSE;
    image_t* ret = NULL;

    const char* path = path_join(DATADIR, filename);
    if (path == NULL)
        goto exit;
    
    f = fopen(path, "rb");
    if (f == NULL)
       goto exit;

    image_header_t img_header;
    if (!img_read_header(&img_header, f))
       goto exit;

    ret = malloc(sizeof(image_t) + 8*img_header.data_size);
    if (ret == NULL)
        goto exit;

    if (!img_read (ret, f))
        goto exit;

    success = TRUE;
exit:
    if (f != NULL)
        fclose(f);
    
    if (!success)
    {
        free(ret);
        ret = NULL;
    }
    
    return ret;
}

# ? Apr 27, 2013 22:42

Room Temperature: Oct 16, 2007; mildly amusing

Funny how even after going to the trouble of using the single run loop as a ghetto goto for a cleanup block they still managed to miss some things to clean up. Well, without seeing the implementation of path_join I can't be sure, but it seems likely that path needs to be freed at some point.

# ? Apr 28, 2013 02:41

ExcessBLarg!: Sep 1, 2001

Suspicious Dish posted:

C++ code:

...
    do {
        ...
        success = TRUE;
    } while (FALSE);

    if (!success && ret != NULL) {
        ...

This guy likes Pascal right?

# ? Apr 28, 2013 16:16

mjau: Aug 8, 2008

Foxfire_ posted:

The sensible thing is just to use gotos for the control flow instead of making a funnylooking goto out of do...while and break:

No need for gotos or breaks

code:

image_t* img_load_from_filename (const char *filename)
{
    FILE* f = NULL;
    image_t* ret = NULL;
    const char* path;
    image_header_t img_header;

    if (!(path = path_join(DATADIR, filename))
        || !(f = fopen(path, "rb"))
        || !img_read_header(&img_header, f)
        || !(ret = malloc(sizeof(image_t) + 8 * img_header.data_size))
        || !img_read(ret, f))
    {
        free(ret);
        ret = NULL;
    }

    if (f)
        fclose(f);

    return ret;
}

(Yes, I'm the horror, etc)

# ? Apr 30, 2013 02:49

shrughes: Oct 11, 2008; (call/cc call/cc)

Side effects? Not in my conditionals.

# ? Apr 30, 2013 02:51

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

My favorite part is the segfault when one of the early conditionals fails. It's the little touches...

# ? Apr 30, 2013 03:32

That Turkey Story: Mar 30, 2003

mjau posted:

No need for gotos or breaks

code:

image_t* img_load_from_filename (const char *filename)
{
    FILE* f = NULL;
    image_t* ret = NULL;
    const char* path;
    image_header_t img_header;

    if (!(path = path_join(DATADIR, filename))
        || !(f = fopen(path, "rb"))
        || !img_read_header(&img_header, f)
        || !(ret = malloc(sizeof(image_t) + 8 * img_header.data_size))
        || !img_read(ret, f))
    {
        free(ret);
        ret = NULL;
    }

    if (f)
        fclose(f);

    return ret;
}

(Yes, I'm the horror, etc)

For some reason, the thing that bothers me the most here is just the lack of use of De Morgan's Law.

# ? Apr 30, 2013 04:28

hobbesmaster: Jan 28, 2008

Do while false really isn't that horrific a construct as weird C things go. Still grimace every time I see it used but I can't get too worked up about it.

That malloc line is a classic C horror though.

# ? Apr 30, 2013 04:43

Foxfire_: Nov 8, 2010

Suspicious Dish posted:

My favorite part is the segfault when one of the early conditionals fails. It's the little touches...

It looks technically correct (the best kind of correct) to me. The conditionals are required to short circuit out as soon as one is true, so the later ones never run.

# ? Apr 30, 2013 04:46

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

Huh, I thought free(NULL); would segfault, but it seems it's entirely safe. I didn't know.

# ? Apr 30, 2013 04:48

hobbesmaster: Jan 28, 2008

Suspicious Dish posted:

Huh, I thought free(NULL); would segfault, but it seems it's entirely safe. I didn't know.

Why do something like 90% of C and C++ programmers not know how malloc, new, free and delete work?

# ? Apr 30, 2013 04:53

Suspicious Dish: Sep 24, 2011; 2020 is the year of linux on the desktop, bro; Fun Shoe

I know how malloc and free work, I just didn't expect free to allow NULL as a special case, since to me it's an invalid input (you can't free a null pointer after all) and the C library typically just says "if you pass in an invalid input, it's your fault". But perhaps the utility of having unguarded frees is so convenient that they put it in there.

# ? Apr 30, 2013 05:00

Volte: Oct 4, 2004; woosh woosh

Suspicious Dish posted:

I know how malloc and free work, I just didn't expect free to allow NULL as a special case, since to me it's an invalid input (you can't free a null pointer after all) and the C library typically just says "if you pass in an invalid input, it's your fault". But perhaps the utility of having unguarded frees is so convenient that they put it in there.

You can also realloc a null pointer and you can free a pointer by reallocing it to zero.

# ? Apr 30, 2013 05:07

That Turkey Story: Mar 30, 2003

Suspicious Dish posted:

I know how malloc and free work, I just didn't expect free to allow NULL as a special case, since to me it's an invalid input (you can't free a null pointer after all) and the C library typically just says "if you pass in an invalid input, it's your fault". But perhaps the utility of having unguarded frees is so convenient that they put it in there.

Yeah. In your defense, I do agree that it's opposite of what you would expect from C (and delete in C++). Undefined behavior for a null pointer would be more in line with the design philosophy.

# ? Apr 30, 2013 05:08

Scaevolus: Apr 16, 2007

Coding horror: a C++ code base where the author doesn't realize delete, like malloc, works with nulls. He then proceeds to manage all his memory manually, with null checks around the deletes.

# ? Apr 30, 2013 07:36

QuarkJets: Sep 8, 2008

Scaevolus posted:

Coding horror: a C++ code base where the author doesn't realize delete, like malloc, works with nulls. He then proceeds to manage all his memory manually, with null checks around the deletes.

Ehhh, not realizing that delete works with nulls isn't a horror. Maybe you could describe some of the manual memory management? Is it particularly horrible or is the null checking the part that you find horrible?

# ? Apr 30, 2013 08:59

hobbesmaster: Jan 28, 2008

Suspicious Dish posted:

I know how malloc and free work, I just didn't expect free to allow NULL as a special case,

code:

MALLOC(3)                BSD Library Functions Manual                MALLOC(3)

NAME
     calloc, free, malloc, realloc, reallocf, valloc -- memory allocation

SYNOPSIS
     #include <stdlib.h>

     void *
     calloc(size_t count, size_t size);

     void
     free(void *ptr);

     void *
     malloc(size_t size);

     void *
     realloc(void *ptr, size_t size);

     void *
     reallocf(void *ptr, size_t size);

...
DESCRIPTION
     The malloc(), calloc(), valloc(), realloc(), and reallocf() functions
     allocate memory.  The allocated memory is aligned such that it can be
     used for any data type, including AltiVec- and SSE-related types.  The
     free() function frees allocations that were created via the preceding
     allocation functions.

     The malloc() function allocates size bytes of memory and returns a
     pointer to the allocated memory.
...
    The free() function deallocates the memory allocation pointed to by ptr.
     If ptr is a NULL pointer, no operation is performed.

Two sentences.

# ? Apr 30, 2013 15:53

Scaevolus: Apr 16, 2007

QuarkJets posted:

Ehhh, not realizing that delete works with nulls isn't a horror. Maybe you could describe some of the manual memory management? Is it particularly horrible or is the null checking the part that you find horrible?

When this repeats for 20 members:

code:

if (mMember1 != NULL) {
    delete[] mMember1;
    }
if (mMember2 != NULL) {
    delete mMember2;
    }
...

It's pretty ugly and unnecessary.

# ? Apr 30, 2013 16:14

JetsGuy: Sep 17, 2003; science + hockey
=
LASER SKATES

Scaevolus posted:

When this repeats for 20 members:
code:
if (mMember1 != NULL) {
    delete[] mMember1;
    }
if (mMember2 != NULL) {
    delete mMember2;
    }
...
It's pretty ugly and unnecessary.

argh, one quick function would have solved all that.

# ? Apr 30, 2013 17:05

evensevenone: May 12, 2001; Glass is a solid.

Isn't mMembers 1 through 20 is the real horror?

Or perhaps codebases that feel the need to delete something that possibly doesn't exist?

# ? Apr 30, 2013 17:20

hobbesmaster: Jan 28, 2008

evensevenone posted:

Isn't mMembers 1 through 20 is the real horror?

Or perhaps codebases that feel the need to delete something that possibly doesn't exist?

I'd hope that that is for example.

(also: use smart pointers)

# ? Apr 30, 2013 17:24

Rottbott: Jul 27, 2006; DMC

evensevenone posted:

Or perhaps codebases that feel the need to delete something that possibly doesn't exist?

Nothing wrong with that. A smart pointer itself being an example.

# ? Apr 30, 2013 18:49

awesmoe: Nov 30, 2005; Pillbug

At the place I used to work, people couldnt be arsed reading the delete documentation so they made a template function to delete pointers

code:

template <typename T>
delete_pointer<T x> {
    if (x != NULL) {
        delete x;
    }
}

Of course, the same people who couldnt be arsed reading the standard library docs sure as hell couldn't be arsed reading our own docs, so people used it like so

code:

if (y != NULL){
    delete_pointer(y);
}

# ? Apr 30, 2013 21:04

Jewel: May 2, 2009

awesmoe posted:

At the place I used to work, people couldnt be arsed reading the delete documentation so they made a template function to delete pointers
code:
template <typename T>
delete_pointer<T x> {
    if (x != NULL) {
        delete x;
    }
}
Of course, the same people who couldnt be arsed reading the standard library docs sure as hell couldn't be arsed reading our own docs, so people used it like so
code:
if (y != NULL){
    delete_pointer(y);
}

Three checks if the pointer is null. Gotta really check hard. Just in case. It's the most important part of any program I'll have you know! One time I didn't check if a pointer was null and my computer completely destroyed itself.

# ? Apr 30, 2013 21:11

Pilsner: Nov 23, 2002

I simply can't fathom how large C++ applications and 3D games can even function, given how insanely complex C/C++ is, notably with regards to memory allocation. My hat goes off (or condolences?) to those to can make it work. Does anyone actually think C++ is fine and dandy, or would you/they prefer a more modern language with GC, a simple and useful standard library, etc., if they had the choice?

# ? Apr 30, 2013 21:27

awesmoe: Nov 30, 2005; Pillbug

Pilsner posted:

I simply can't fathom how large C++ applications and 3D games can even function, given how insanely complex C/C++ is, notably with regards to memory allocation. My hat goes off (or condolences?) to those to can make it work. Does anyone actually think C++ is fine and dandy, or would you/they prefer a more modern language with GC, a simple and useful standard library, etc., if they had the choice?

GC is frustrating and unpredictable and limited, RAII is much, much simpler than GC, c++ is fine wrt memory management as long as people don't write code like it's 1994. The standard library is incredibly limited but they're working on it.

# ? Apr 30, 2013 21:45

Adbot: ADBOT LOVES YOU

# ? May 17, 2024 20:17

Bruegels Fuckbooks: Sep 14, 2004; Now, listen - I know the two of you are very different from each other in a lot of ways, but you have to understand that as far as Grandpa's concerned, you're both pieces of shit! Yeah. I can prove it mathematically.

Pilsner posted:

I simply can't fathom how large C++ applications and 3D games can even function, given how insanely complex C/C++ is, notably with regards to memory allocation. My hat goes off (or condolences?) to those to can make it work. Does anyone actually think C++ is fine and dandy, or would you/they prefer a more modern language with GC, a simple and useful standard library, etc., if they had the choice?

Well one of the reasons you would use C++ is because you don't want GC if you're writing stuff that performs well.

It isn't that hard to make stuff not leak - if you're calling new or free or malloc this day an edge and aren't working on embedded software, you're doing poo poo wrong. Smart pointers and referenced counted smart pointers can handle everything for you, but the problem is that everyone has their own convention - your company needs to pick a convention for how to deal with memory allocation, have tools that enforce this convention on check in, and you need to run static analysis tools.

# ? Apr 30, 2013 21:46

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1503 »