Conversation
[why] It is probably a leftover from the auto-conversion of GUL14. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
[why] Both the OverloadSet class template and the make_overload_set() function template were included in GUL14, but removed along with the variant.h header in the transition to GUL17. However, at least the class template is still useful in conjunction with std::visit(). [how] Import both the class and the function template. Rewrite OverloadSet in C++17 style with a deduction guide so it can be constructed directly from an arbitrary list of lambdas. Deprecate the make_overload_set() function because it no longer offers any benefit over instantiating an OverloadSet directly. Import the original tests for make_overload_set(). Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
[why]
The function call is equivalent to constructing an OverloadSet{...} in
place.
Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
[why] The header is no longer related to std::variant in any way. It only contains the OverloadSet type. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
[why] The Catch2 header was already removed from GUL17 much earlier, and the OverloadSet.h header file should be included with gul.h like almost all other header files. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
[why] We should document that we have re-introduced the OverloadSet class template into the library. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
alt-graph
requested review from
Finii,
Seim2k17,
Copilot,
ohensler and
soerengrunewald
There was a problem hiding this comment.
Pull Request Overview
This PR reintroduces the OverloadSet class template in its own header, updates the library’s umbrella and build scripts to include it, and adds corresponding unit tests.
- Add
OverloadSet.hwith the class template and C++17 deduction guide - Register the new header in
meson.buildand include it ingul.h - Add tests in
tests/test_OverloadSet.ccand update build configurations - Remove a stale
traits.h_file and update changelog indata/doxygen.h
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_OverloadSet.cc | Add unit tests for the new OverloadSet behavior |
| tests/meson.build | Register test_OverloadSet.cc in the test suite |
| include/gul17/traits.h_ | Remove legacy traits.h_ file |
| include/gul17/meson.build | Add OverloadSet.h to the list of standalone headers |
| include/gul17/gul.h | Include OverloadSet.h in the main umbrella header and bump year |
| include/gul17/OverloadSet.h | New header defining OverloadSet and its deduction guide |
| data/doxygen.h | Document the reintroduction of OverloadSet in the changelog |
Comments suppressed due to low confidence (4)
tests/test_OverloadSet.cc:2
- The Doxygen \file annotation shows
test_variant.ccinstead oftest_OverloadSet.cc. Update it to match the actual filename.
* \file test_variant.cc
include/gul17/OverloadSet.h:2
- The Doxygen \file annotation should refer to
OverloadSet.hinstead ofvariant.h.
* \file variant.h
include/gul17/OverloadSet.h:24
- The header guard macro should be renamed to
GUL17_OVERLOADSET_H_to matchOverloadSet.h.
#ifndef GUL17_VARIANT_H_
include/gul17/OverloadSet.h:3
- [nitpick] The header comment mentions
make_overload_set(), but no such function is defined here. Either remove this reference or add the deprecated function implementation.
* \brief Implementation of the OverloadSet class template and the make_overload_set() function template.
[why] Both should follow the renaming of the file to OverloadSet.h. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
[why] It should follow the renaming of the file to test_OverloadSet.h. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
[why] The function has been removed from the file. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
There was a problem hiding this comment.
What is the removed file traits.h_ 🤔 🤷♀️
This looks 100% like the example on cppreference.com, which given the extreme small size of piece of code, may be coincidence, or you actually took that.
Maybe mention something like 'taken from' or 'insopied by' if the code actiually came from somewhere else.
[why] The original design of GUL14's OverloadSet class was inspired by the "overloaded" class proposed in that book. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
I think I originally got the idea for GUL14 from "A Tour of C++", and the class from the cppreference article looks very much like that one. But then... there are not many ways to skin that cat. In any case, I have added a reference to Bjarne's book. |
This pull request reintroduces and updates the
OverloadSetclass template which had been part of GUL14 and was thrown out semi-accidentally with the removal of thevariant.hheader. It is still useful for use withstd::visit(), and therefore it is reimported, updated, and moved into a new header fileOverloadSet.h.Discussion points:
make_overload_set(). With C++17 template argument deduction guides, this function has lost its reason for existing. In the first commit, it is still present but deprecated. We could decide to keep it that way, but I prefer a clean break.OverloadSetinvariant.has it was in GUL14 for compatibility? I guess not,OverloadSet.his a cleaner name.