DM-29563: replace std::variant, fix clang compilation #581
Conversation
mwittgen
force-pushed
the
tickets/DM-29563
branch
3 times, most recently
from
eb8a749
to
abeb6e4
Compare
| return boost::get<T const&>(foo); | ||
| } catch (boost::bad_get const&) { | ||
| return std::get<std::reference_wrapper<T const>>(foo); | ||
| } catch (std::bad_variant_access const&) { |
There was a problem hiding this comment.
Do we still need to use exceptions at all, now that we have holds_alternative?
There was a problem hiding this comment.
I'd guess not; just something that didn't occur to me.
There was a problem hiding this comment.
Change to use holds_alternative.
| @@ -434,7 +433,7 @@ class GenericMap { | |||
| message << "Key " << key << " not found, but a key labeled " << key.getId() << " is present."; | |||
| throw LSST_EXCEPT(pex::exceptions::OutOfRangeError, message.str()); | |||
| } | |||
| } catch (boost::bad_get const&) { | |||
| } catch (std::bad_variant_access const&) { | |||
There was a problem hiding this comment.
Consider replacing with holds_alternative.
| F _func; | ||
| public: | ||
|
|
||
| explicit refwrap_visitor(F && func) : _func(std::move(func)) {} |
There was a problem hiding this comment.
Should this be std::forward?
There was a problem hiding this comment.
Probably, and even more probably with C++17's adjustments to construction that allows class template constructors to behave more like function templates. But I would need to get my head back into this much more than I have right now to be certain.
There was a problem hiding this comment.
Change to std::forward?
| // Generic case: unwrap the reference_wrapper, then copy-construct and | ||
| // cast to Python. |
There was a problem hiding this comment.
Sorry it took me so long to get to this.
But I started looking at the clang-tidy changes, and almost every one I've seen is actually undesirable for one reason or another (or at least questionable) - usually it's a case where our code is doing something that isn't exactly best practice, but fixing it naively (as clang-tidy has done) is simply incorrect.
The only exceptions so far are the changes from iterator-based to range-based for; those do look okay.
I think it might be best to remove those clang-tidy commits from this ticket entirely, and perhaps run it much more carefully (or at least less aggressively) on a future ticket. Were any of them needed to get the variant and filesystem changes compiling at all?
mwittgen
force-pushed
the
tickets/DM-29563
branch
from
cddf6fb
to
74a02b9
Compare
|
@TallJimbo removed all the clang-tidy stuff and also the cherry-pick for arm-osx compilation for now as is changes the compile flag -D_LIBCPP_DISABLE_AVAILABILITY=1 for all platforms and all the arm osx stuff should go into a proper ticket. |
There was a problem hiding this comment.
I've responded to @kfindeisen 's comments on the code that I wrote. But I wouldn't be able to actually get to addressing them in the code myself for at least a few weeks, I'm afraid, so I'm hoping @mwittgen can take that on.
The code that I didn't originally write (just adding std:: to nullptr_t, now, I think) is approved.
@mwittgen , did you review the std::variant changes in afw::table? It doesn't look like @kfindeisen did, from where his comments are.
| return boost::get<T const&>(foo); | ||
| } catch (boost::bad_get const&) { | ||
| return std::get<std::reference_wrapper<T const>>(foo); | ||
| } catch (std::bad_variant_access const&) { |
There was a problem hiding this comment.
I'd guess not; just something that didn't occur to me.
| F _func; | ||
| public: | ||
|
|
||
| explicit refwrap_visitor(F && func) : _func(std::move(func)) {} |
There was a problem hiding this comment.
Probably, and even more probably with C++17's adjustments to construction that allows class template constructors to behave more like function templates. But I would need to get my head back into this much more than I have right now to be certain.
include/lsst/afw/table/types.h
Outdated
| template <typename ...E> struct TypeList {}; | ||
|
|
||
| /// A compile-time list of scalar field types. | ||
| using ScalarFieldTypes = TypeList<AFW_TABLE_SCALAR_FIELD_TYPES>; |
There was a problem hiding this comment.
ScalarFieldTypes is never used.
| try { | ||
| auto foo = unsafeLookup(key.getId()); | ||
| auto foo = unsafeLookup(key.getId()); | ||
| if(std::holds_alternative<std::reference_wrapper<T const>>(foo)) { |
There was a problem hiding this comment.
Our style has a space between if and (.
Switch from boost::variant to std::variant in afw::table. fix clang compilation
No description provided.