#1332 PR merged: Unite calculations in partition code.¶
Labels: enhancement, documentation, cleanup,
fixed / solved / done
gozora opened issue at 2017-04-25 08:05:¶
As discussed in #1307, this patch updates arithmetical calculations in partition code.
gozora commented at 2017-04-25 10:34:¶
Thanks @jsmeix,
Can we then rename calculate => float_calculate (or maybe
calculate_float)?
Another option would be to have one calculate function and control type
of calculation with function parameter, like:
calculate ("int")
calculate ("float")
?
V.
jsmeix commented at 2017-04-25 10:55:¶
float_calculate (or calculate_float) are misleading
because the result of that function is not float.
I do not know a meaningful name for a function that
calculates internally with basically unlimited precision
and only output its result as integer.
Therefore I avoided possibly misleading additions
of data types in the function name and leave it
to the poor programmer to find out on his own
what that beast actually does ;-)
jsmeix commented at 2017-04-26 08:45:¶
@gozora your
https://github.com/rear/rear/pull/1332/commits/5f61f1a553da6df778774794fb105b199e919451
is a good example why one cannot "just always"
use the calculate function and this shows that
the generic name "calculate" is inappropriate
because it makes that function look as if it
could be "simply used".
Therefore I think a non-generic name is needed
to make programmers aware to have a closer look
because there is something special with that function.
Hereby I propose a name that is more specific
but without any data types in the function name:
precision_calculate
The name is derived from what "man bc" shows topmost:
NAME
bc - An arbitrary precision calculator language
gozora commented at 2017-04-26 08:54:¶
Hereby I propose a name that is more specific
but without any data types in the function name:precision_calculate
I agree with your proposal and adding mine ;-).
Another name that could make programmer to "stop and think" before use
could be:
mathlib_calculate ()
Derived from bc -l:
-l, --mathlib
Define the standard math library.
V.
jsmeix commented at 2017-04-26 09:02:¶
@gozora
I agree with you: mathlib_calculate ()
makes it even more obvious that this one
is not "just normal integer calculation".
Could you perhaps also add some explanatory comment
to the mathlib_calculate function code, in particular this
example here that mathlib_calculate "7 % 4" results 0
instead of the integer calculation remainder 3.
jsmeix commented at 2017-04-26 09:43:¶
@gdha
please do not yet merge it until @gozora
has changed it to the new mathlib_calculate
function name.
jsmeix commented at 2017-04-26 11:22:¶
Using mathlib_calculate for things like
start_mb=$( mathlib_calculate "$start / 1024 / 1024" )
seems to work o.k. because
# echo '123 * 1024 * 1024' | bc 128974848 # mathlib_calculate "128974848 / 1024 / 1024" 123 # echo '1024 * 1024 -1' | bc 1048575 # echo '123 * 1024 * 1024 + 1048575' | bc 130023423 # mathlib_calculate "130023423 / 1024 / 1024" 123 # mathlib_calculate "130023424 / 1024 / 1024" 124
gozora commented at 2017-04-26 11:26:¶
I think for simple calculations as in ...
How will be define "simple" calculations ?
Like: +; - ; * ?
In other words where floats can't be result ?
V.
gozora commented at 2017-04-26 11:29:¶
The comment in the mathlib_calculate function description
must be ..
Therefore one cannot use the mathlib_calculate function
Thanks!
jsmeix commented at 2017-04-26 11:33:¶
@gozora
what do you think:
Should mathlib_calculate be used everywhere where possible
or should bash arithmetic still be used when no multiplication
happens?
Simple sums and differences bash can calculate up to 2^63 - 1
which is sufficient for disks up to 8 EiB minus one byte.
Only when a multiplication happens
the values are limited in practice up to 2^31 - 1
(i.e. then it fails for disks with 2 GiB and more).
Cf.
https://github.com/rear/rear/issues/1269#issuecomment-297321546
jsmeix commented at 2017-04-26 11:36:¶
@gozora
with "simple calculations" I meant only sums and differences.
mathlib_calculate must be used when there is at least one
multiplication.
gozora commented at 2017-04-26 11:38:¶
In my opinion we should unify ALL calculations (or all new calculations
at least) except modulo under mathlib_calculate ().
That would avoid lots of problems with limitations of awk or bash
arithmetic expansions, or I don't know what else.
V.
jsmeix commented at 2017-04-26 11:42:¶
FWIW:
I really dislike code like
end="$(($end-1))B"
that changes the data type of a variable by the way
which results some additional "WTF" while wondering
"WTF does that 'end' thingy actually contain"...
gozora commented at 2017-04-26 11:43:¶
Apart from
https://github.com/rear/rear/pull/1332#issuecomment-297371922,
it will be easier to define development guideline like:
"For all computations except modulo use mathlib_calculate()"
is better then
"For sums and differences use mathlib_calculate()".
The first one looks more clear to me ...
V.
gozora commented at 2017-04-26 11:47:¶
Yes, I've noticed
end="$(($end-1))B"
code, but as I did not had time to study it deeper, I just did lazy
transfer to new function ...
Until it doesn't make trouble, I'll not poke into it ;-)
V.
jsmeix commented at 2017-04-26 11:53:¶
@gozora
I fully agree with you so that I "just merged" it now.
Many thanks for your patience with this
unexpectedly complicated issue!
Isn't it the most primitive use case for a computer
to "just calculate" (correctly?)
;-)
gozora commented at 2017-04-26 12:14:¶
No problem.
Hopefully we've found method that is universal enough!
V.
gozora commented at 2017-04-26 12:27:¶
@gdha, @jsmeix, @schlomo, @dagwieers, @jhoekx
Would anybody mind if I add to following to ReaR
wiki:
For all mathematical calculations except modulo (%) use mathlib_calculate()
(see #1332 for more details)
?
V.
jsmeix commented at 2017-04-26 13:07:¶
@gozora
in general fine with me.
But I would not use the strict "all ... except modulo"
because I think there are other valid exceptions.
I would keep it more neutral like
the description in global-functions.sh
already is - something like
* mathlib_calculate()
For mathematical calculations use mathlib_calculate()
unless strict integer calculation is required, see
usr/share/rear/lib/global-functions.sh how to use it.
For example instead of using bash arithmetic
result=$(( 2**32 * 2**32 ))
that fails because numbers get too big for bash use
result=$( mathlib_calculate "2^32 * 2^32" )
gozora commented at 2017-04-28 06:40:¶
schlomo commented at 2017-05-08 14:08:¶
I just added bc to the RPM/DEB dependencies, unfortunately I cannot test this everywhere.
schlomo commented at 2017-05-08 14:08:¶
@gdha can you fix the OBS Snapshot builds? I'll then test it where I can.
gdha commented at 2017-05-08 14:14:¶
@schlomo I pushed a rebuild - you can follow the progress via https://build.opensuse.org/package/show?package=rear&project=Archiving:Backup:Rear:Snapshot
schlomo commented at 2017-05-08 15:36:¶
Thanks. is there no automated trigger for that?
gdha commented at 2017-05-08 15:48:¶
yes of course - every night it will run (if required)
On Mon, May 8, 2017 at 5:36 PM, Schlomo Schapiro
notifications@github.com
wrote:
Thanks. is there no automated trigger for that?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rear/rear/pull/1332#issuecomment-299903028, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA2POSYKWBMe5NFDgCFm9vDhxFAQF3nIks5r3zaBgaJpZM4NHIZY
.
schlomo commented at 2017-05-09 15:30:¶
I validated the build on both CentOS 7.3 and Ubuntu 16.4, don't have SUSE any more so maybe @jsmeix can check it. I used the snapshot builds from http://software.opensuse.org//download.html?project=Archiving%3ABackup%3ARear%3ASnapshot&package=rear
jsmeix commented at 2017-05-09 15:40:¶
@schlomo
no need to worry about SUSE.
I am using newest ReaR GitHub master code all the time
on my test systems which is nowadays mainly SLES12
and from time to time I may even test on SLES11.
But I cannot test all possibilities with reasonable effort.
ProBackup-nl commented at 2017-05-10 21:39:¶
@gdha In my opinion this is not a quality merge.
Because of a bug in a speed optimised variant (mawk) of a generic
tool, the generic (awk) tool is ditched completely and replaced by a
completely different tool. Not knowing which new issues that will be
introduced.
Better revert to the old awk code, rename that to limit running only
gawk (because GNU awk hasn't got these large integer calculation
issues), and catch the remaining issues (for example an mawk only
source OS) with additional code/tools/new dependencies.
jsmeix commented at 2017-05-11 08:34:¶
See
https://github.com/rear/rear/issues/1269#issuecomment-300719707
[Export of Github issue for rear/rear.]