6
\$\begingroup\$

I am a programmer with a primarily Rust-based background who has nonetheless taken an interest in C lately due to its simplicity and minimalism. However, one of the things I miss most from Rust is safe memory management. So I created a "smart pointer" of sorts to replicate that experience:

#include <stdio.h>

/* 
This is a "smart" pointer that
uses Option types for safe memory management.
*/

typedef enum option {
    Some,
    None
} Option;

typedef struct OptionPointer {
    void *content;
    Option state;
    int is_malloc;
} OptionPointer;

union internal_PointerType {
    void *content;
    int err;
};

typedef struct PointerType {
    int choice;
    union internal_PointerType internal;
} PointerType;

OptionPointer create_ptr(void *content, int is_malloc) {
    OptionPointer p;
    p.content = content;
    p.state = Some;
    p.is_malloc = is_malloc;
    return p;
}

void set_ptr_invalid(OptionPointer *ptr) {
    ptr->state = None;
    // Only need to free memory
    // if it was dynamically allocated,
    // otherwise we only need to worry
    // about dangling pointers
    if (ptr->is_malloc) {
        free(ptr->content);
    }
}

PointerType get_ptr(OptionPointer *ptr) {
    PointerType res;
    res.choice = 0;
    
    if (ptr->state == None) {
        res.choice = 0;
        res.internal.err = 1;
    } else {
        res.choice = 1;
        res.internal.content = ptr->content;
    }
    return res;
}

// Example
int main() {
    char *a = "This is some testing text";
    OptionPointer ptr = create_ptr(a, 0);
    
    // Imaginary scenario where the pointer becomes invalid
    set_ptr_invalid(&ptr);
    
    PointerType res = get_ptr(&ptr);
    if (res.choice) {
        char *content = (char*)res.internal.content;
        printf("%s\n", content);
    } else {
        printf("Invalid pointer at %s:%d\n", __FILE__, __LINE__);
    }
    
}

I can already see some notable problems:

  • I literally just started programming in C a few days ago so I probably am not using recommended best practices
  • "Smart" only goes so far - you'd have to call set_ptr_invalid() every single time a pointer could become invalid as a result of an operation
  • This (possibly?) might not work well with malloc() and doesn't cover the case that malloc() wasn't successful

All feedback on this would be much appreciated!

\$\endgroup\$
8
  • 3
    \$\begingroup\$ 1) There is an implicit conversion to and from a void * in C. You don't need to cast a void * to a char *. 2) Use bool, true, and false from stdbool.h to denote a binary state. \$\endgroup\$
    – Harith
    Dec 21, 2023 at 14:13
  • 1
    \$\begingroup\$ set_ptr_invalid() does not check for the current state and thus is prone to multiple-free bugs if called repeatedly. \$\endgroup\$ Dec 21, 2023 at 14:45
  • \$\begingroup\$ The scariest thing about C is UB. Consider writing non-Rust code in zig instead. There's no such thing as undefined behavior for that compiler. Either it runs the code you wrote, or it fails with a diagnostic. \$\endgroup\$
    – J_H
    Dec 21, 2023 at 23:50
  • 4
    \$\begingroup\$ The purpose of smart pointers is automatic freeing of memory. You cannot do that in C. \$\endgroup\$ Dec 22, 2023 at 2:04
  • 1
    \$\begingroup\$ @J_H ziglang.org/documentation/master/#Undefined-Behavior "Zig has many instances of undefined behavior." You cannot have a low-level language without UB at some point. \$\endgroup\$ Dec 22, 2023 at 20:04

2 Answers 2

11
\$\begingroup\$

Disclaimer: The next few lines are my opinion as a C programmer so take it with a grain of salt.

As a C programmer, I will never use this. The language is verbose enough already due to its lack of features. You are now erasing the type of the pointer by stuffing it in void making my job harder. Take the following example.

void do_something(PointerType * some_object);

What is the underlying type of some_object? There is no way of telling without documentation. If some idiot stuffs the wrong underlying type. I have a nice time figuring out access violations.

Let us say it is MyStruct.

typedef struct MyStruct {
    char character;
    int integer;
} MyStruct;

If I was implementing do_something. I could just do it like this with regular pointers.

void do_something(MyStruct * some_object){
    if(some_object == NULL){
        printf("Invalid pointer at %s:%d\n", __FILE__, __LINE__);
        return;
    }

    printf("Value of my struct is {character : %c, integer : &d}", some_object->character, some_object->integer);
}

With your code I have to first get the underlying type and then cast it to MyStruct*, it adds quite a few lines.

OptionPointer also adds an overhead of an enum to my pointer. And is going potentially confuse the compiler optimizer generating suboptimal code.

Review

  1. Add a check for NULL in create_ptr. Someone might easily stuff a NULL in there. This would also mean you have to have an error mechanism if create_ptr fails.
  2. Use opaque ptr paradigm to hide the internals of OptionPointer otherwise there is no way of preventing a C programmer from just treating this as a struct and modify state and contents directly.
  3. Using opaque ptr also means that you will have to add a function to check the value of choice.
  4. There is only 1 way a pointer fails aka it is NULL. So you do not need a separate err code for that.

This is how I would write the same thing.

#include <stdio.h>
#include <stdbool.h>
/*---------- Pointer.h --------------*/
typedef struct Pointer Pointer;

Pointer create_ptr(void * ptr);
void set_ptr_invalid(Pointer *ptr);
bool is_ptr_valid(Pointer* ptr);
void* get_ptr(Pointer* ptr);

/*---------- Pointer.c ------------*/
typedef struct Pointer {
    void* content;
} Pointer;

Pointer create_ptr(void * ptr){
    Pointer p;
    p.content = ptr;
    return p;
}

void set_ptr_invalid(Pointer *ptr) {
    ptr->content = NULL;
}

bool is_ptr_valid(Pointer* ptr){
    return ptr->content != NULL;
}

void* get_ptr(Pointer* ptr){
    return ptr->content;
}

/*------------ Main.c ---------- */

// Example
int main() {
    char const * a = "This is some testing text";
    Pointer ptr = create_ptr((void*)a);
    
    if (is_ptr_valid(&ptr)) {
        char *content = (char*)get_ptr(&ptr);
        printf("%s\n", content);
    } else {
        printf("Invalid pointer at %s:%d\n", __FILE__, __LINE__);
    }

    set_ptr_invalid(&ptr);

    if (is_ptr_valid(&ptr)) {
        char *content = (char*)get_ptr(&ptr);
        printf("%s\n", content);
    } else {
        printf("Invalid pointer at %s:%d\n", __FILE__, __LINE__);
    }
}
```
\$\endgroup\$
8
\$\begingroup\$

It isn't possible to implement smart pointers sensibly in C because it lacks "RAII". It would be possible to implement a garbage collector by multi-threading, but one of the benefits of using C in the first place is that it doesn't have garbage collection. So essentially you are just creating a superfluous "bloat" layer here, which really doesn't add anything. Additionally you lose type safety - normal pointers in C are quite safe, but void* are not. So overall, this code is not a good idea.

Ignoring the purpose of the program, here is a review on the pure C programming aspects:

  • Whenever we use a pointer to a string literal such as char *a = "This is some testing text";, it should pretty much always be declared as const char*, because modifying a string literal invokes undefined behavior. The only time when you wouldn't use const char* is a scenario where you would later want to re-assign the pointer to read/write memory.
  • is_malloc isn't really necessary because if something is allocated with malloc, it either points at an address or is a null pointer. Therefore we can use the data pointer and check it vs null to tell if something was allocated. You already do that: ptr->content != NULL.
  • Similarly free(ptr) is safe as long as the pointer either points at an address previously returned by malloc (& friends) or in case it is a null pointer. It is also good practice to set the pointer to NULL after calling free().
  • Your struct and union usage style is inconsistent. In case of the union you suddenly don't use typedef. Also, in this program as-is, you don't need struct tags but could just do typedef struct {.
  • Passing structs by value isn't really recommended since it is often slow. Your struct here is small so it's not a big deal, but the rule of thumb is to always pass structs by pointer.

Note that writing generic abstraction layers in C is hard and it's a common mistake by intermediately experienced programmers to produce a lot of such, which later on turns out to have added nothing but increased complexity, bloat and worse performance.

Personally I still have to maintain some very old code I wrote fresh out of school and it's filled with such mistakes. I can understand what I was thinking back then: generic programming and abstraction would lead to more code re-use and reduced code repetition. But it turned out that the application using the code did not need abstraction layers and type-generic programming at all, it's just a burden for the poor soul which has to maintain it (often yourself, later on).

Instead focus on the actual requirements of the specific project. If you later down the line spot an opportunity to simplify the program by adding abstraction or code re-use, then consider implementing it at that point. Excellent programmers strive to make programs simpler, bad programmers strive to make them more complex.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ I wish the advice in the last three paragraphs was more often heeded.... \$\endgroup\$ Dec 22, 2023 at 14:15
  • 1
    \$\begingroup\$ This is gold. Thank you. \$\endgroup\$
    – WiiLF
    Dec 23, 2023 at 18:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.