Make macros::expand_numeric return int64_t #3454
Conversation
|
The question is what to do with rpmExpandNumeric() that uses this interface. It needs to have defined behavior in case of overflow, but there's no way to return an error from rpmExpandNumeric(). |
|
I suppose the least dangerous option is to emit a warning and cap the return value to int min/max. |
ffesti
force-pushed
the
expandnumeric
branch
from
c70c940
to
8fca2a2
Compare
rpmio/macro.cc
Outdated
| } else if (val < INT_MIN) { | ||
| rpmlog(RPMLOG_WARNING, _("Macro value too small for int: %" PRIu64 " Using %i instead.\n"), val, INT_MIN); | ||
| res = INT_MIN; | ||
| } else { |
There was a problem hiding this comment.
Turns out there's an STL solution for this: https://en.cppreference.com/w/cpp/algorithm/clamp
No point having separate messages for too small and too big, just state that it's outside range.
ffesti
force-pushed
the
expandnumeric
branch
from
8fca2a2
to
295bde7
Compare
|
Make one wonder if there isn't an expand_macro_to_numeric function in the STL... |
rpmio/macro.cc
Outdated
| if (arg == NULL) | ||
| return 0; | ||
| auto [ ign, val ] = macros().expand_numeric(arg); | ||
| return val; | ||
| res = std::clamp(val, (int64_t) INT_MIN, (int64_t) INT_MAX); |
There was a problem hiding this comment.
Why the casts? It shouldn't need them, AFAICS.
There was a problem hiding this comment.
No idea why you ask me.
The compiler says
/home/ffesti/CVS/rpm/rpmio/macro.cc: In function ‘int rpmExpandNumeric(const char*)’:
/home/ffesti/CVS/rpm/rpmio/macro.cc:2001:21: error: no matching function for call to ‘clamp(std::tuple_element<1, std::pair<int, long int> >::type&, int, int)’
2001 | res = std::clamp(val, INT_MIN, INT_MAX);
| ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/14/algorithm:61,
from /home/ffesti/CVS/rpm/rpmio/macro.cc:13:
/usr/include/c++/14/bits/stl_algo.h:3623:5: note: candidate: ‘template<class _Tp> constexpr const _Tp& std::clamp(const _Tp&, const _Tp&, const _Tp&)’
3623 | clamp(const _Tp& __val, const _Tp& __lo, const _Tp& __hi)
| ^~~~~
/usr/include/c++/14/bits/stl_algo.h:3623:5: note: template argument deduction/substitution failed:
/home/ffesti/CVS/rpm/rpmio/macro.cc:2001:21: note: deduced conflicting types for parameter ‘const _Tp’ (‘long int’ and ‘int’)
2001 | res = std::clamp(val, INT_MIN, INT_MAX);
| ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/14/bits/stl_algo.h:3643:5: note: candidate: ‘template<class _Tp, class _Compare> constexpr const _Tp& std::clamp(const _Tp&, const _Tp&, const _Tp&, _Compare)’
3643 | clamp(const _Tp& __val, const _Tp& __lo, const _Tp& __hi, _Compare __comp)
| ^~~~~
/usr/include/c++/14/bits/stl_algo.h:3643:5: note: candidate expects 4 arguments, 3 provided
make[2]: *** [rpmio/CMakeFiles/librpmio.dir/build.make:132: rpmio/CMakeFiles/librpmio.dir/macro.cc.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:681: rpmio/CMakeFiles/librpmio.dir/all] Error 2
make: *** [Makefile:136: all] Error 2
otherwise. What ever that means.
There was a problem hiding this comment.
Right... Any time one gets delusioned to think something simple like this would "just work", c++ is merry to prove you wrong 😆
It really is a huge change in thinking to first check if the language can do something for you on a given problem. With C, you could always trust it wont 😄 |
rpmio/macro.cc
Outdated
| @@ -1992,10 +1994,15 @@ rpmExpand(const char *arg, ...) | |||
| int | |||
| rpmExpandNumeric(const char *arg) | |||
| { | |||
| int res; | |||
There was a problem hiding this comment.
Guess I just didn't notice this before, but don't introduce uninitialized variables. Nothing needs res until it gets the value from the clamp, so just introduce it there.
rpmio/macro.cc
Outdated
| @@ -9,6 +9,8 @@ | |||
| #include <string> | |||
| #include <vector> | |||
| #include <stack> | |||
| #include <cinttypes> | |||
| #include <algorithm> | |||
There was a problem hiding this comment.
I've been mostly trying to keep the C++ includes to alphabetic order, simply because that's such a blissfully simple rule and then you don't need to think about it. The pre-existing includes in this file are obviously failing to do so, but lets try to stick to that whenever there's no reason to deviate from it. Mind, that's not documented anywhere, we need to really start collecting the new rules somewhere....
ffesti
force-pushed
the
expandnumeric
branch
from
295bde7
to
25979f1
Compare
While we can't change the C API there is really no reason to add a new 32 bit API. Changing now while it is still internal. Emit a warning if value doesn't fit into an int for the old API and clamp at INT_MIN and INT_MAX.
ffesti
force-pushed
the
expandnumeric
branch
from
25979f1
to
0566e31
Compare
|
Just realized we missed a test for this. Oh well... |
While we can't change the C API there is really no reason to add a new 32 bit API. Changing now while it is still internal.