Remove extraneous casts to void #4377
Conversation
There was a problem hiding this comment.
Overall this looks straightforward. The only bit was when I read up on std::ignore and the docs say that it's behavior outside of std::tie is undefined not specified with a caveat that some folks are using it exactly as you are:
While the behavior of std::ignore outside of std::tie is not formally specified, some code guides recommend using std::ignore to avoid warnings from unused return values of [[nodiscard]] functions.
I don't know C++ enough to know whether the undefined unspecified bit is important, but I just recently fixed a UBSAN issue so it stuck out to me. Given that none of those changes are in public headers, I'm inclined to consider it fine if all of our CI runners pass.
+1. Will approve once CI is green.
| @@ -1205,11 +1201,7 @@ ByteVecValue Dimension::map_from_uint64( | |||
|
|
|||
| template <> | |||
| ByteVecValue Dimension::map_from_uint64<char>( | |||
There was a problem hiding this comment.
Change Dimension* to Dimension&?
There was a problem hiding this comment.
Out of scope for this PR. I've done some of that kind of thing before, and it tends to cascade throughout. That kind of change is best left to its own change set, where you can make all the changes needed in their own package.
| @@ -201,11 +201,7 @@ uint64_t Domain::cell_num_per_tile() const { | |||
|
|
|||
| template <> | |||
| int Domain::cell_order_cmp_impl<char>( | |||
| const Dimension* dim, const UntypedDatumView a, const UntypedDatumView b) { | |||
| // Must be var-sized | |||
| assert(dim->var_size()); | |||
There was a problem hiding this comment.
Not a big deal as I'm planning to change a lot of these implementations to std::string to prevent exactly this soon. This check might prevent someone using this function to process a CHAR datatype.
There was a problem hiding this comment.
In this case this check was not only unnecessary but not even the quite the right check.
There was a problem hiding this comment.
What would be the right check?
This PR removes a number of extraneous casts to void, though not all in the same way. It does not have a goal of removing all of extraneous casts. The most frequent by far is to prevent unused variable warnings for function arguments that are not actually used. In this case changing to an anonymous argument solves the problem. It also indicates quite clearly that an argument isn't used. In some cases checks for the validity of unused arguments were also removed. In some cases a variable declaration (argument or otherwise) were annotated with `[[maybe_unused]]`. This was most often the case where the code varies with preprocessor definitions. In a few cases some argument checks were promoted from `assert` to throwing exceptions when invalid. A handful of ignored return values were replaced with `std::ignore`.
eric-hughes-tiledb
force-pushed
the
eh/removed_unused_arguments
branch
from
947b9bc to
6f6e517
Compare
…te that other compilers use, so I patted it on the head and told it go play nicely with others.
…causes warnings in that particular circumstance.
…t are now very much used. clang-format
This PR removes a number of extraneous casts to void, though not all in the same way. It does not have a goal of removing all of extraneous casts.
The most frequent by far is to prevent unused variable warnings for function arguments that are not actually used. In this case changing to an anonymous argument solves the problem. It also indicates quite clearly that an argument isn't used. In some cases checks for the validity of unused arguments were also removed.
In some cases a variable declaration (argument or otherwise) were annotated with
[[maybe_unused]]. This was most often the case where the code varies with preprocessor definitions. In a few cases some argument checks were promoted fromassertto throwing exceptions when invalid..TYPE: NO_HISTORY
DESC: Remove extraneous casts to void