inexorgame / vulkan-renderer

A new 3D game engine for Linux and Windows using C++20 and Vulkan API 1.3, in very early but ongoing development
https://inexor.org
MIT License
815 stars 34 forks source link

Mention C++ code guideline rules which are important for the project in Inexor's documentation #208

Closed IAmNotHanni closed 3 years ago

IAmNotHanni commented 4 years ago

The C++ Code Guidelines are amazing! I will peak through them in the next weeks and sort out the ones which are useful. This selection will ofc be based on my experience, so please add to this. Once we have the list, I would to add it to the documentation. Also, I want to link this list directly on the start page of the repo.

As a result of this analysis, many refactoring tickets will be created to enforce the selected rules.

IAmNotHanni commented 4 years ago

P.1: Express ideas directly in code Every loop which iterates through a vector e.g. to find a certain value can be replaced with std::find:

bool AvailabilityChecksManager::has_instance_extension(const std::string &extension_name) {
    assert(!extension_name.empty());

    if (instance_extensions_cache.empty()) {
        create_instance_extensions_cache();
    }

    // Loop through all available instance extensions and search for the requested one.
    for (const VkExtensionProperties &instance_extension : instance_extensions_cache) {
        // Compare the name of the current instance extension with the requested one.
        if (strcmp(instance_extension.extensionName, extension_name.c_str()) == 0) {
            // Yes, this instance extension is supported!
            return true;
        }
    }

    // No, this instance extension could not be found and thus is not supported!
    return false;
}

becomes:

bool AvailabilityChecksManager::has_instance_extension(const std::string &extension_name) {
    assert(!extension_name.empty());

    if (instance_extensions_cache.empty()) {
        create_instance_extensions_cache();
    }

    if(std::find(instance_extensions_cache.begin(), instance_extensions_cache.end(), extension_name.c_str()) != instance_extensions_cache.end()) {
        return true;
    }

    return false;
}

Again, I think I can shorten some variable names in the code :)

IAmNotHanni commented 4 years ago

P.5: Prefer compile-time checking to run-time checking:

"You don’t need to write error handlers for errors caught at compile time." "Don’t postpone to run time what can be done well at compile time." Good example from the Framegraph PR:

/// @brief Adds either a render resource or render stage to the frame graph
/// @return A mutable reference to the just-added resource or stage
template <typename T, typename... Args>
T &add(Args &&... args) {
    auto ptr = std::make_unique<T>(std::forward<Args>(args)...);
    auto &ret = *ptr;
    if constexpr (std::is_base_of_v<RenderResource, T>) {
        m_resources.push_back(std::move(ptr));
    } else if constexpr (std::is_base_of_v<RenderStage, T>) {
        m_stages.push_back(std::move(ptr));
    } else {
        static_assert(!std::is_same_v<T, T>, "T must be a RenderResource or RenderStage");
    }
    return ret;
}

Instead of checking if the framegraph is valid at runtime, we can do so at compile-time.

IAmNotHanni commented 4 years ago

P.6: What cannot be checked at compile time should be checkable at run time:

"(...), we should endeavor to write programs that in principle can be checked, given sufficient resources (analysis programs, run-time checks, machine resources, time)."

IAmNotHanni commented 4 years ago

P.7: Catch run-time errors early:

"Avoid “mysterious” crashes. Avoid errors leading to (possibly unrecognized) wrong results."

IAmNotHanni commented 4 years ago

P.8: Don’t leak any resources:

"Even a slow growth in resources will, over time, exhaust the availability of those resources. This is particularly important for long-running programs, but is an essential piece of responsible programming behavior."

"A leak is colloquially anything that isn’t cleaned up. The more important classification is anything that can no longer be cleaned up.

Use RAII (Resource Acquisition Is Initialization) ! "Postconditions of the form “this resource must be released” are best expressed by RAII."

See also: Resource Management.

IAmNotHanni commented 4 years ago

P.12: Use supporting tools as appropriate.

IAmNotHanni commented 4 years ago

P.13: Use support libraries as appropriate: "A widely used library is more likely to be kept up-to-date and ported to new systems than an individual application."

I personally like this one a lot: "You need a reason not to use the standard library (or whatever foundational libraries your application uses) rather than a reason to use it."

IAmNotHanni commented 4 years ago

I.1: Make interfaces explicit:

IAmNotHanni commented 4 years ago

I.3: Avoid singletons:

IAmNotHanni commented 4 years ago

I.10: Use exceptions to signal a failure to perform a required task:

IAmNotHanni commented 4 years ago

I.11: Never transfer ownership by a raw pointer or reference:

IAmNotHanni commented 4 years ago

F.2: A function should perform a single logical operation: