Changes to support C++14 and fixed missing include file (#501) #503
Conversation
| // An alternative spelling of the default constructor because void isn't regular | ||
| // and C++14 requires a copy-ctor even when it must be elided. This allows us to | ||
| // "manually" elide the copy-ctor. | ||
| priority_task_system(std::nullptr_t) : priority_task_system() {} |
There was a problem hiding this comment.
Terrible comment if we're playing by Dave's rules. What does it do? Most constructors should be // Creates … and then you say something about the properties of the instance you get. What are the properties of a default-constructed priority_task_system?
All of that other justification should go in a paragraph outside the summary sentence fragment, and probably not even in the doc comment. I can't tell whether it's implementation notes or what. Oh… it belongs in the place where you use this constructor, because the whole immediately-called lambda thing needs explanation. It took me about 5 minutes to figure that out; I was looking for a ctor that takes a lambda…
| @@ -429,11 +434,9 @@ class priority_task_system { | |||
| }; | |||
|
|
|||
| inline priority_task_system& pts() { | |||
There was a problem hiding this comment.
Could you please add a comment describing the semantics of this function? I can't evaluate whether your changes are correct without it.
| only_task_system.join(); | ||
| }); | ||
| return priority_task_system{}; | ||
| static priority_task_system only_task_system{[] { |
There was a problem hiding this comment.
Here's where you explain the nullptr-taking ctor.
| only_task_system.join(); | ||
| }); | ||
| static task_system<P> only_task_system{[] { | ||
| at_pre_exit([]() noexcept { only_task_system.join(); }); |
There was a problem hiding this comment.
whitespace-only changes are unkind to reviewers unless the review tool has a way to ignore them. Github has that option, but I think it doesn't include newlines. FWIW.
There was a problem hiding this comment.
We have a .editorconfig file and .clangformat file in the project - but not everyone sets their editor to use them. Annoying.
| @@ -26,10 +26,12 @@ class copy_on_write { | |||
| struct model { | |||
| std::atomic<std::size_t> _count{1}; | |||
|
|
|||
| model() noexcept(std::is_nothrow_constructible_v<T>) = default; | |||
| model() noexcept(std::is_nothrow_constructible<T>::value) = default; | |||
There was a problem hiding this comment.
I presume you're following some guideline/convention that's documented somewhere?
There was a problem hiding this comment.
Required for C++14 - _v is a C++17 addition.
| @@ -60,8 +62,8 @@ class copy_on_write { | |||
| copy_on_write(U&& x, disable_copy<U> = nullptr) : _self(new model(std::forward<U>(x))) {} | |||
|
|
|||
| template <class U, class V, class... Args> | |||
| copy_on_write(U&& x, V&& y, Args&&... args) | |||
| : _self(new model(std::forward<U>(x), std::forward<V>(y), std::forward<Args>(args)...)) {} | |||
| copy_on_write(U&& x, V&& y, Args&&... args) : | |||
There was a problem hiding this comment.
I like the old version better ;-)
Improving comments per code review.
Fixed issue in portable default_executor requiring a copy-ctor for an elided construction. Issue #501
Added missing config.hpp include in optional.hpp which is required for C++14 to get boost optional.
Changed
_vtraits in copy_on_write.hpp to use::valueinstead for C++14.All C++14 unit tests now pass on macOS with the portable executor.