Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use C++20 source_location feature in exceptions #473

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IAmNotHanni
Copy link
Member

Closes #468

Source locations!

C++20 gives us a very nice new feature, std::source_location!

VulkanException(std::string message, VkResult result,
                std::source_location location = std::source_location::current());
VulkanException::VulkanException(std::string message, const VkResult result,
                                 const std::source_location location)
    : InexorException(message.append(" (")
                          .append(vk_tools::as_string(result))
                          .append(": ")
                          .append(vk_tools::result_to_description(result))
                          .append(") (")
                          .append("file: ")
                          .append(location.file_name())
                          .append(", line: ")
                          .append(std::to_string(location.line()))
                          .append(", column: ")
                          .append(std::to_string(location.column()))
                          .append(", function name: ")
                          .append(location.function_name())
                          .append(")")) {}

Throwing an example exception from rendergraph:

throw VulkanException("I am throwing a new exception!", VK_ERROR_INCOMPATIBLE_DRIVER);

Results in:

terminate called after throwing an instance of 'inexor::vulkan_renderer::VulkanException'
what(): I am throwing a new exception! (VK_ERROR_INCOMPATIBLE_DRIVER: The requested version of Vulkan is not supported by the driver or is otherwise incompatible for implementation-specific reasons) (file: /home/johannes/Inexor/exception_refactoring/vulkan-renderer/src/vulkan-renderer/render_graph.cpp, line: 85, column: 89, function name: void inexor::vulkan_renderer::RenderGraph::build_buffer(const inexor::vulkan_renderer::BufferResource&, inexor::vulkan_renderer::PhysicalBuffer&) const)

(Note that I didn't add a catch block, so the exception will not be caught properly. However this was only a proof of concept)

This will make debugging a lot easier!

@IAmNotHanni IAmNotHanni added cat:enhancement enhancement/requested feature/update of existing features cat:refactor refactor/clean up/simplifications/etc. labels Jun 19, 2022
@IAmNotHanni IAmNotHanni requested review from yeetari and IceflowRE June 19, 2022 12:03
@IAmNotHanni IAmNotHanni self-assigned this Jun 19, 2022
@IAmNotHanni
Copy link
Member Author

Oh it looks like clang isn't ready for source_location yet.

@IAmNotHanni IAmNotHanni changed the title [exception] Use source location feature Use C++20 source_location feature in exceptions Jun 19, 2022
@IAmNotHanni IAmNotHanni added the org:on hold on hold, until ... label Jun 26, 2022
class InexorException : public std::runtime_error {
public:
// No need to define own constructors.
// There is no need to define our own constructors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// There is no need to define our own constructors
// There is no need to define our own constructors.

#include <stdexcept>
#include <string>

namespace inexor::vulkan_renderer {

/// @brief A custom base class for exceptions
/// A custom base class for exceptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// A custom base class for exceptions
/// A custom base class for exceptions.

Comment on lines +21 to +27
/// Default constructor
/// Here we are using `C++20 source location feature <https://en.cppreference.com/w/cpp/utility/source_location>`__
/// @param message The exception message
/// @param result The VkResult value of the Vulkan API call which failed
/// @param location The source location
VulkanException(std::string message, VkResult result,
std::source_location location = std::source_location::current());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Default constructor
/// Here we are using `C++20 source location feature <https://en.cppreference.com/w/cpp/utility/source_location>`__
/// @param message The exception message
/// @param result The VkResult value of the Vulkan API call which failed
/// @param location The source location
VulkanException(std::string message, VkResult result,
std::source_location location = std::source_location::current());
/// Default constructor.
/// @param message The exception message.
/// @param result The VkResult value of the Vulkan API call which failed.
/// @param location The source location.
VulkanException(std::string message, VkResult result,
std::source_location location = std::source_location::current());

I would not especially point out the new C++20 feature.

Comment on lines +7 to +20
VulkanException::VulkanException(std::string message, const VkResult result, const std::source_location location)
: InexorException(message.append(" (")
.append(vk_tools::as_string(result))
.append(": ")
.append(vk_tools::result_to_description(result))
.append(") (")
.append("file: ")
.append(location.file_name())
.append(", line: ")
.append(std::to_string(location.line()))
.append(", column: ")
.append(std::to_string(location.column()))
.append(", function name: ")
.append(location.function_name())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are using C++20 we could use std::format here.

@BrettDong
Copy link

Clang 15 now supports std::source_location.

@IAmNotHanni
Copy link
Member Author

Note for myself: The exception class can be made [[nodiscard]]

@IAmNotHanni IAmNotHanni added prio:low This has low priority. and removed cat:enhancement enhancement/requested feature/update of existing features labels May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:refactor refactor/clean up/simplifications/etc. org:on hold on hold, until ... prio:low This has low priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use std::source_location in exceptions
3 participants