ad hoc logging in c++ projects

Question

I am currently going through this Vulkan tutorial.

An extra excercise was writing a function which checks if the hardware you are running on supports the extensions other libraries require (GLFW in this case).

All the code is here:

#define GLFW_INCLUDE_VULKAN
#include <GLFW/glfw3.h>

#include <iostream>
#include <stdexcept>
#include <cstdlib>
#include <vector>
#include <unordered_set>
#include <functional>

const uint32_t WIDTH = 800;
const uint32_t HEIGHT = 600;

enum Verbosity : bool {
    VERBOSE = true,
    TERSE = false,
};

//todo: enumerate the available extensions, print them
//      and check if the ones required by glfw are in that list
class HelloTriangleApplication {
public:
    void run() {
        initWindow();
        initVulkan();
        mainLoop();
        cleanup();
    }

private:
    GLFWwindow* window;
    VkInstance instance;

    void initWindow() {
        glfwInit();

        glfwWindowHint(GLFW_CLIENT_API, GLFW_NO_API);
        glfwWindowHint(GLFW_RESIZABLE, GLFW_FALSE);

        window = glfwCreateWindow(WIDTH, HEIGHT, "vulkan",
                                  nullptr, nullptr);

    }

    void initVulkan() {
        createInstance();

    }

    void createInstance() {
        VkApplicationInfo appInfo{};
        appInfo.sType = VK_STRUCTURE_TYPE_APPLICATION_INFO;
        appInfo.pApplicationName = "Hello Triangle";
        appInfo.applicationVersion = VK_MAKE_VERSION(1, 0, 0);
        appInfo.pEngineName = "No Engine";
        appInfo.engineVersion = VK_MAKE_VERSION(1, 0, 0);
        appInfo.apiVersion = VK_API_VERSION_1_0;

        VkInstanceCreateInfo createInfo{};
        createInfo.sType = VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO;
        createInfo.pApplicationInfo = &appInfo;

        uint32_t glfwExtensionCount = 0;
        const char ** glfwExtensions;

        glfwExtensions = glfwGetRequiredInstanceExtensions(&glfwExtensionCount);

        try {
            checkRequiredExtensions(std::vector<std::string>(glfwExtensions, glfwExtensions + glfwExtensionCount),
                                    VERBOSE);
        } catch (std::exception &e) {
            throw;
        }

        createInfo.enabledExtensionCount = glfwExtensionCount;
        createInfo.ppEnabledExtensionNames = glfwExtensions;
        createInfo.enabledLayerCount = 0;

        if (vkCreateInstance(&createInfo, nullptr, &instance) != VK_SUCCESS) {
            throw std::runtime_error("failed to create instance!");
        }
    }

    void checkRequiredExtensions(std::vector<std::string> requiredExtensions, const Verbosity &&verbose) {
        if (verbose) {
            std::cerr << "Required extensions: ";
            for (auto &extension: requiredExtensions)
                std::cerr << extension << ", ";
            std::cerr << '\n';
        }

        uint32_t extensionCount = 0;
        vkEnumerateInstanceExtensionProperties(
                nullptr, &extensionCount, nullptr);

        std::vector<VkExtensionProperties> supportedExtensions(extensionCount);
        vkEnumerateInstanceExtensionProperties(nullptr,
                           &extensionCount, supportedExtensions.data());

        std::unordered_set<std::string> supportedExtensionNames;
        std::transform(supportedExtensions.begin(), supportedExtensions.end(),
                       std::inserter(supportedExtensionNames, supportedExtensionNames.begin()),
                       [](const auto& extension) {
            return extension.extensionName;
        });

        if (verbose) {
            std::cerr << "Available extensions: ";
            for (auto &extension: supportedExtensionNames) {
                std::cerr << extension << ", ";
            }
            std::cerr << '\n';
        }

        if(std::any_of(requiredExtensions.begin(), requiredExtensions.end(),
                [&](const auto& extension) {
            return !supportedExtensionNames.contains(extension);
        })) {
            throw std::runtime_error("Hardware does not support the required Vulkan extensions");
        }

        if (verbose) {
            std::cerr << "Hardware supports required extensions\n";
        }
    }

    void mainLoop() {
        while(!glfwWindowShouldClose(window)) {
            glfwPollEvents();
        }
    }

    void cleanup() {
        vkDestroyInstance(instance, nullptr);

        glfwDestroyWindow(window);

        glfwTerminate();
    }
};

int main() {
    HelloTriangleApplication app;

    try {
        app.run();
    } catch (const std::exception& e) {
        std::cerr << e.what() << std::endl;
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

I'm specifically interested in the code I wrote in void checkRequiredExtensions(std::vector<std::string> requiredExtensions, const Verbosity &&verbose).

Am I handling exceptions correctly? Have I chosen a good way of selecting the verbosity mode of the function?

I find graphics programs super hard to debug so like logging everything and want the best way to do this.

I'm also worried about runtime overhead (as those if conditions need to be evaluated multiple times). Is there a way to avoid this? Am I making any dumb mistakes?

Answer 1

Write clear and concise error messages

You will notice that almost any program will not log anything when things are fine. That's desired, because you don't want to be distracted by information that is not useful. When things go wrong though, you want to get a message that explains exactly what went wrong, without overloading you with details that do not matter.

So if all the required extensions are present, you should not log anything. If an extension is missing, you should log exactly which extension(s) you are missing. So what I would write is something like:

void checkRequiredExtensions(const std::vector<std::string> &requiredExtensions) {
    // Get list of supported extensions
    ...
    
    std::vector<std::string> missingExtensions;

    for (extension: requiredExtensions) {
        if (supportedExtensionNames.contains(extension)) {
            missingExtensions.push_back(extension);
        }
    }

    if (!missingExtensions.empty()) {
        std::cerr << "Missing required Vulkan extensions:\n"

        for (auto extensions: missingExtensions) {
             std::cerr << "- " << extension << "\n";
        }

        throw std::runtime_error("missing required Vulkan extensions");
    }
}

Now I no longer have to manually compare the list of required extensions with available extensions to find out what is missing. It also avoids the need for a verbosity level.

When you catch an exception, do something useful or don't catch at all

The following code is not doing anything useful:

try {
    checkRequiredExtensions(...);
} catch (std::exception &e) {
    throw;
}

This is equivalent to not using try...catch at all. Also in main() you write:

try {
    app.run();
} catch (const std::exception& e) {
    std::cerr << e.what() << std::endl;
    return EXIT_FAILURE;
}

Here what you are doing in the catch-block is almost identical to what would happen if you didn't catch the exception at all, since uncaught exceptions are printed and then the program is aborted with a non-zero exit code.

Avoid premature optimization

I would not create a std::unordered_set here, you should only have to check supported extensions once at the start of the program, so efficiency is much less important here than maintainability. So I would write:

uint32_t extensionCount = 0;
vkEnumerateInstanceExtensionProperties(nullptr, &extensionCount, nullptr);

std::vector<VkExtensionProperties> supportedExtensions(extensionCount);
vkEnumerateInstanceExtensionProperties(nullptr, &extensionCount, supportedExtensions.data());

std::vector<std::string> missingExtensions;

for (auto extension: supportedExtensions) {
    if (std::find(requiredExtensions.begin(), requiredExtensions.end(), extension.extensionName) == requiredExtensions.end()) {
        missingExtensions.push_back(extension.extensionName);
    }
}

If you really feel performance is critical here, then I would still not create a std::unordered_set, but rather std::sort() supportedExtensions, and use std::binary_search() to do efficient lookups. This avoids creating yet another container. And if you can ensure requiredExtensions is also sorted, you can avoid std::binary_search() as well and just do a linear scan through both sorted vectors.

Consider writing checkMissingExtensions() instead

It would be a bit cleaner to write a function named checkMissingExtensions() that just returns a vector with the missing extensions. This way, the function does not have to throw exceptions or write error messages, this can then be done by the caller. This also gives the caller more flexibility; for example if you want to use a certain extension but have fall-back code if it isn't present, then you can also use this function without an error message being printed unconditionally.

Debugging Vulkan programs

While you are developing your Vulkan application, you should have validation layers enabled. These will provide you with error messages when you are using the API incorrectly.

Answer 2

Here are some general comments and some things that may help you improve your program.

Consider whether this is the right API for you

Vulkan is a very powerful but very difficult and verbose API to use.

I find graphics programs super hard to debug so like logging everything and want the best way to do this.

If you find graphics programs super hard to debug, Vulkan is going to make your life even more difficult. Consider getting up to speed with something a little easier to master such as OpenGL.

With that said, almost all of the code in the question is verbatim from the tutorial, so I will restrict my review solely to the checkRequiredExtensions call you created.

Pass by value for small parameters

The code currently declares the function like so:

void checkRequiredExtensions(std::vector<std::string> requiredExtensions, const Verbosity &&verbose);

The second argument does not need to be a move reference. Just pass it by value.

void checkRequiredExtensions(std::vector<std::string>& requiredExtensions, const Verbosity verbose);

See this question for an more detailed explanation.

Consider the use of std::copy

Printing the extensions is currently done like this:

for (auto &extension: requiredExtensions)
    std::cerr << extension << ", ";
std::cerr << '\n';

That's not terrible (although I would use const auto& instead), but you might also consider using std::copy for this:

std::copy(requiredExtensions.begin(), requiredExtensions.end(), 
        std::ostream_iterator<std::string>(std::cerr, ", "));

If you want to get fancy and omit the final ,, see if your compiler and libraries support std::experimental::ostream_joiner.

Also, note that you could print the required extensions directly using copy:

std::copy(glfwExtensions, &glfwExtensions[glfwExtensionCount],
        std::ostream_iterator<const char *>(std::cerr, ", "));

Check for errors

The calls to vkEnumerateInstanceExtensionProperties can fail, so you should check for a return value of VK_SUCCESS at the very least.

Simplify by avoiding copies

The code gathers a vector of VkExtensionProperties but then copies it into an unordered set. There's not much point to that, since you could simply print directly from the vector:

std::cerr << "Available extensions: ";
for (auto &extension: supportedExtensions) {
    std::cerr << extension.extensionName << ", ";
}
std::cerr << '\n';

Understand the API

The code to check for the presence of required extensions is not really required. The way Vulkan works is that the required extensions are enumerated explicitly:

createInfo.enabledExtensionCount = glfwExtensionCount;
createInfo.ppEnabledExtensionNames = glfwExtensions;

When vkCreateInstance is called, it will return VK_ERROR_EXTENSION_NOT_PRESENT if any of the enumerated extensions are missing.

Answer 3

The use of classes and its member functions seems wrong to me:

• Destructor should also clean up if your code throws. In case initVulkan throws, your cleanup will never be reached and you get a resource leak. Also, having a destructor makes sure that when HelloTriangleApplication app goes out of scope, i.e. when main ends, all resources are cleaned-up without explicitly calling any special function.

• The public member function doing everything (construction, destruction, capture events) is bad design. It should do one thing.

---

Put initWindow() into the constructor of HelloTriangleApplication: HelloTriangleApplication().

Put cleanup() into the destructor of HelloTriangleApplication: ~HelloTriangleApplication().

Put initVulkan() (which only calls create_instance()) and mainLoop() into a public function like check_compat.

Delete run().

Make checkRequiredExtensions a non-member function. Since it doesn't access any private members of the Class, it shouldn't be a member function. Also this will let you write tests for it.

---

Use const in cases like:

auto &extension : supportedExtensionNames
std::exception &e

---

Adding to the point about exception G Sliepen raised,

When you catch an exception, do something useful or don't catch at all

Add extra info to the exception if you need it later on and then throw it. See more details at

• https://stackoverflow.com/questions/2360597/c-exceptions-questions-on-rethrow-of-original-exception

---

Use std::cout and std::cerr when need arises. Don't stream everything to std::cerr. One reason is to conveniently separate stdout and stderr later on if this program is run via command-line.

Another reason is the use of std::endl. All errors should be visible in stderr right when they occur. So use std::endl. And this is not required in std::cout since those messages are not the ones you want to log.