Custom Error Handler Library in C++

Question

Is this code clean and optimized? When taking memory snapshots, it seems a bit high. Anything I have done wrong or can do to optimize it?

#pragma once
#include <iostream>
#include <windows.h>
#include <sstream>
#include <Psapi.h>

// Author: https://github.com/TheRacc2
// Other Help: https://github.com/dgoguerra/, for the bytes_to_size function (modified from the "humanSize" function found in https://gist.github.com/dgoguerra/7194777)
// Repository: https://github.com/TheRacc2/error_handler/
// Report issues here: https://github.com/TheRacc2/error_handler/issues

constexpr const char* name = "Error Handler";

namespace helpers {
    // Convert bytes into readable sizes, E.G: 1024B -> 1KB, 1024000B -> 1MB
    //
    // call as such:
    // const char* sign = bytes_to_size(1024);
    const char* bytes_to_size(int bytes) {
        const char* units[] = { "B", "KB", "MB", "GB", "TB" };
        const int size = sizeof(units) / sizeof(units[0]) - 1;

        int i = 0;
        double outside_bytes = bytes;

        if (bytes > 1024)
            for (; (bytes / 1024) > 0 && i < size; i++, bytes /= 1024)
                outside_bytes = bytes / 1024;

        static char final[12];
        snprintf(final, 12, "%.01f %s", outside_bytes, units[i]);
        return final;
    }
}

namespace error_handler {
    // Use this in a "try, catch" situation, the send to developers function doesn't need to be provided.
    // 
    // call as such:
    // try {
    //     your_function_or_code();
    // } CATCH(send_log_to_developer(ex))
    #define CATCH(send_to_developers_function) \
    catch (const std::exception& ex) { \
        std::stringstream error; \
        error << ex.what(); \
        error << "\n\n"; \
        error << "Would you like to report this to the developers? Should your request be seen, this crash may be prevented."; \
        \
        if (MessageBoxA(nullptr, error.str().c_str(), name, MB_YESNO) == IDYES) { \
            send_to_developers_function; \
            MessageBoxA(nullptr, "Thank you for reporting this!", name, MB_OK); \
        } \
    }

    // Call this when something needs to go wrong, formatted like "printf". The second argument doesn't need to be provided.
    // 
    // call as such:
    // THROW("You didn't use this right! You used this like: %s", "throw()")
    // 
    // output:
    // "You didnt use this right! You used this like: throw()"
    #define THROW(text, ...) \
    error_handler::throw_ex(text, __FILE__, __FUNCTION__, __LINE__, __VA_ARGS__);

    // You shouldn't manually call this function, as you have to provide the file, function and line.
    // Use the "THROW" macro instead.
    template <typename... Args>
    void throw_ex(const char* text, const char* file, const char* function, const int& line, Args... args) {
        char message[128];
        snprintf(message, 128, text, args...);

        std::stringstream stream;
        stream << message << "\n\n";

        PROCESS_MEMORY_COUNTERS_EX pmc;
        K32GetProcessMemoryInfo(GetCurrentProcess(), reinterpret_cast<PROCESS_MEMORY_COUNTERS*>(&pmc), sizeof(pmc));

        stream <<
            "{\n" <<
            "    File: \"" << file << "\",\n"
            "    Function: \"" << function << "\",\n"
            "    Line: " << line << ",\n"
            "    Memory: \"" << helpers::bytes_to_size(pmc.PrivateUsage) << "\"\n"
            "}"
        ;

        throw std::exception(stream.str().c_str());
    }
}

Answer 1

Make bytes_to_size() reentrant

Since bytes_to_size returns a pointer to a static buffer, it is not reentrant. This will be a problem if you have multiple threads throwing exceptions at the same time. There are two solutions:

1. Have bytes_to_sign() take a pointer to a buffer as an argument, and have it write the text into that.
2. Have it return a std::string.

Avoid mixing C and C++ string formatting

It's very weird to see both snprintf() and std::stringstream used at the same time. Now you will get the worst of both worlds: type unsafety and possible truncation from snprintf(), and the overhead of dynamically growing memory from std::stringstream. Pick one and stick with it.

Consider using std::format

C++20 introduced std::format, which gives you type-safe string formatting. If you want to be compatible with older versions of C++, you can use the {fmt} library.

About units

The SI prefixes are powers of 1000, if you want to make sure people are not confused, use the IEC binary prefixes, so KiB, MiB, GiB and so on. I would also add all the defined prefixes, so up to YiB.

Consider allowing different exception types to be thrown

Your THROW() adds helpful information to the exception message, but the exception type is fixed to std::exception. That means that it is hard for any code that wants to catch exceptions to determine what exactly went wrong has very little information. Maybe you can add the exception type as an argument to the macro?

#define THROW(type, text, ...) \
    error_handler::throw_ex<type>(text, __FILE__, __FUNCTION__, __LINE__, __VA_ARGS__);

template <typename Exception, typename... Args>
void throw_ex(const char* text, ...) {
    ...
    throw Exception(stream.str().c_str());
}

Then it can be used like so:

if (value > MAX_VALUE) {
    THROW(std::out_of_range, "value %d larger than maximum %d", value, MAX_VALUE);
}

C++20 will avoid the need for macros

C++20 introduces std::source_location which avoids the need to use macros to get the filename and line number at the place where the exception is thrown.

Answer 2

Range Limits

Your function's description says "1024B -> 1KB", but that doesn't work, you have an off-by-one error in your implementation. This also suggests that you haven't unit-tested your code. BTW: I think you have a one-off error as well, because size is not the number of elements in the array, but one less. The number of elements is idiomatically computed by (sizeof array)/(sizeof array[0]).

Another issue with those range limits in your code is that int is a signed type and you're completely ignoring negative values. Additionally, you're converting the number to a floating-point type, which causes a loss of precision.

Scaling Factor

One thing you've not gotten right is the prefixes for the different sizes. The correct ones should be ki, Mi, Gi etc. Alternatively, use the scaling factors of 1000, not 1024. In any case, the prefix k (1000) is not written with capital letters! More in-depth info can be found in Wikipedia.

Non-Standard Exception Constructor

If you look at std::exception docs, you will not find a constructor taking a char const*. Your code only works because your compiler ships with a non-standard extension. Instead, use std::runtime_error, which has a constructor taking a string. Then, you can also drop the .c_str() from the expression it's initialized with.

Namespaced Macros

...aren't. Your code

namespace error_handler {
    #define CATCH(send_to_developers_function) \
    ...
}

looks like it's isolating the macro in a namespace, but it doesn't. Macros just don't work like that, so that part is just misleading. Avoid macros in general (I guess you know that), but in this case add a check that this macro isn't already defined. Especially with common identifiers like THROW, collisions with other code are likely.

Magic Numbers

This code

char message[128];
snprintf(message, 128, text, args...);

has two problems. One is that it's repeating the size of the array. Using above way to represent the size would avoid that. The other is probably some kind of cargo cult programming. Using powers of two isn't going to make your code better, faster or more correct. Why not use 100 or 200? How did you pick that number at all? This decision is what costs most time during development, so it is definitely worth documenting! In this case, it's probably just "I guess this is enough", but even that would be helpful to understand the code.