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

Added noexcept for most cases #338

Merged
merged 1 commit into from
Oct 23, 2019
Merged

Conversation

oddlama
Copy link
Contributor

@oddlama oddlama commented Jun 6, 2019

TL;DR: Added noexcept where currently possible, while skipping some (not too important) functions where this could not be easily determined. This is a draft, because there are still two open questions, which are discussed below (destructor of UniqueHandle, and ensuring noexcept correctness on templates).

Introduction

This PR adds VULKAN_HPP_NOEXCEPT (noexcept) to most functions where it should be added. There are still two open questions on edge-cases, which will be discussed below. Therefore, this PR is still a draft.

Remarks

I have compiled a short list of remarks and thoughts about this PR, which you might
want to consider.

[Info] The VULKAN_HPP_NOEXCEPT macro

This macro is necessary, as MSVC prior to version 19 does not support noexcept.
In this draft, there is also VULKAN_HPP_HAS_NOEXCEPT which is only defined if there is
noexcept support, thus allowing the use of #ifdef VULKAN_HPP_HAS_NOEXCEPT.
This is required for more complicated cases, in which a simple noexcept specifier does not suffice, but something along the lines of noexcept(noexcept(expression)) must be used.

[Info] VULKAN_NO_EXCEPTIONS and noexcept

While it was suggested in #165 to apply noexcept to every function if VULKAN_NO_EXCEPTIONS is used, this is not always possible.

There are several functions that return objects like std::vector, which can throw in the copy constructor (guaranteed copy elision exists only from c++17 onwards).
Additionally, the (copy-)constructor of std::tuple is used, as well as std::tuple_cat() and std::make_tuple(), which are all noexcept(false).

While there are some functions (such as createResultValue()) which could be marked noexcept if exceptions are disabled, the current draft of this PR only focuses on those areas, where noexcept can also be applied in the presence of exceptions.

[Info] Implemented cases:

All functions for handles, the Flags template, both dispatch loaders, Optional and ArrayProxy (except front() and back()) have been marked noexcept.

Additionally all functions generated via writeFunction(), that return void have been marked noexcept.
While this is kind of O.K. for now, it misses some cases.
At this point I would like to suggest, that it would be by far better to calculate this information beforehand.

[Open Question] Destructors: UniqueHandle and therefore ObjectDestroy, ObjectFree and PoolFree:

These need some special treatment, and are currently marked noexcept
everywhere except on destroy() and therefore transitively reset() and respectively the destructor of UniqueHandle.

The problem here is, that while most destruction/free functions return void, there are some outliers (e.g. freeDescriptorSets()), that can fail and throw an exception.

ObjectDestroy and ObjectFree have no uses linked to one of those, so technically they can be completely noexcept. But I don't think this assumption should be made, as it is neither future-proof, nor the right way to do it.

PoolFree currently has several free*() functions that return a result, and can therefore throw (in createResultValue). The other two should probably be treated this way, too.

UniqueHandle is only transitively linked to the behavior of the three above, and could use a noexcept(noexcept(expression)) to express this.

Solutions:

  1. Don't mark the destroy functions noexcept
    This can be problematic, as this means that UniqueHandles can never be used with stl containers. (Which was explicitly requested in noexcept for UniqueHandle and handle types? #165)
  2. Mark all of them noexcept, regardless of the actual noexcept-ness
    Allows usage in containers, but is probably even more problematic, because the application will call std::terminate if it happens that an exception is thrown in the destructor.
    Also errors can be missed.
  3. Mark functions conditionally noexcept based on the underlying destroy function
    Allows usage in containers for 99% of the types (which are all actually noexcept),
    but introduces a discrepancy for the remaining types. Trying to use the remaining ones in containers will result in a compile time error.
  4. Something else?

[Open Question] Ensuring correctness (static_assert vs noexcept(condition))

Currently there are some places (StructureChain, ObjectDestroy, ...), where noexcept was added, but one could deliberately instantiate an object where this assumption would not be correct.

Example: StructureChain:
StructureChain is only ever instantiated with types like vk::*Info (or similar), and all of them have noexcept (copy-)constructors. Therefore all functions in StructureChain (except get()) can also be noexcept. Although it is then theoretically possible to instantiate a StructureChain that violates this by throwing in the structs (copy-)constructor.

As there is currently no case similar to this in the project, I would appreciate feedback on how you would like to handle this situation.
Three possible solutions come to my mind:

  1. Just assume noexcept
    This is the currently implemented behavior, as it is the simplest solution, but it has the drawback listed above.
  2. Use dynamic noexcept(noexcept(expression))
    Dynamically changing the noexcept specifier according to the types is possible, but
    might be leading to wrong assumptions about noexcept-ness from developers using Vulkan-Hpp
  3. Use static_assert
    To prevent any instantiations with types that would violate the noexcept assumption.
    Provides errors at compile-time but introduces the requirement for noexcept (copy-)constructible template types

[Info] Minor remarks:

Defaulted virtual destructors have been removed from derived exception
types, as they prevent the generation of implicit move constructors, and are anyway implicitly virtual according to § 15.4 Destructors [class.dtor]:

A destructor can be declared virtual (13.3) or pure virtual (13.4); if any objects of that class or any derived class are created in the program, the destructor shall be defined. If a class has a base class with a virtual destructor, its destructor (whether user- or implicitly-declared) is virtual.

Summary

Currently this draft provides `noexcept´ for all simple cases, and solves #165 when a decision for the two problems above has been made.

I would appreciate your feedback on how the two points are best handled, and also on any other issue you see with this PR. I am happy to address these before "releasing" the PR.

@oddlama
Copy link
Contributor Author

oddlama commented Oct 22, 2019

@asuessenbach @mtavenrath This issue has been waiting for comments from vulkan-hpp maintainers for almost 5 months now.

Your opinion on the topics (design decisions and minor problems) which I have outlined in the description would be greatly appreciated. I am also willing to incorporate the required changes after you have stated your opinion and update this PR accordingly.

@oddlama
Copy link
Contributor Author

oddlama commented Oct 22, 2019

TL;DR: PR is now feature complete.

I have rebased the PR on the current upstream version, and solved the more complex noexcept cases, which I originally described in the remarks.

Wrapper functions

All vk* wrapper functions are noexcept, except when they return a VkResult originally, or the enhanced version uses allocators in the return type.

RAII deleters

All RAII deleters are now always noexcept, as they should be.
Destructors have implicitly been noexcept anyway. There exists one function (vkFreeDescriptorSets) which theoretically can fail, though I am not sure why. I have opened Issue#1070 in Vulkan-Docs to clarify this situation.

For this single function, the deleter can throw - which is problematic as it terminates the program. Nonetheless, it already has been like this anyway (implicitly noexcept destructors), so no additional failure cases were added.

This PR is now feature complete.

Copy link
Contributor

@mtavenrath mtavenrath left a comment

Choose a reason for hiding this comment

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

@nyronium Thank you for changes on adding noexcept to Vulkan-Hpp. Your changes look good and all which is missing to merge this commit is a generated vulkan.hpp.

@mtavenrath
Copy link
Contributor

With regards to the vkFreeDescriptorSets issue, i'd be very surprised if freeing resources will report an out of memory failure. If this turns out to be a real world case we should add some special handling for it, otherwise your solution is the right way to go.

… functions, trivial cases)

This includes complex cases such as generated functions (those not returning VkResult and nothing
with an allocator like std::vector), all internal classes (Flags),
all functions not returning VkResult, as well as all trivial cases.
@oddlama
Copy link
Contributor Author

oddlama commented Oct 23, 2019

@mtavenrath Good to hear that you'd have chosen the same resolutoin. I have updated the PR to include the generated vulkan.hpp.

@mtavenrath mtavenrath merged commit 6da60c5 into KhronosGroup:master Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants