Re: New repo in kdereview: kalk
You probably want to look at https://doc.qt.io/qt-5/qlocale.html#toFloat for converting the numbers from strings to floats. This also probably means that you need to tell Flex to consider 1.000,000 as a single token and find a way to convert it as a float before giving it to Bison. I hope this help, it's a while I didn't use Flex (and I never used Bison). Cheers, Carl Le mardi, mai 4, 2021 7:00 PM, hanyoung a écrit : > Flex doesn't take care of separators, MPFR and GMP do. Flex is merely > scanning for numbers and operators to pass to Bison. > > Sent with ProtonMail Secure Email. > > ‐‐‐ Original Message ‐‐‐ > On Wednesday, May 5, 2021 12:56 AM, Thomas Baumgart t...@net-bembel.de wrote: > > > Han, > > On Dienstag, 4. Mai 2021 11:36:04 CEST hanyoung wrote: > > > > > Pushed an inelegant solution - include "," as decimal separator in flex. > > > As long as there aren't any more decimal separator we're cool. > > > > Not sure if this appropriate, but I wanted to warn you about the dilemma > > that in some locale the thousand separator and the decimal symbol are > > exactly the opposite. Example: > > German locale: 1.000,00 -> one thousand > > US locale : 1,000.00 -> one thousand > > So simply treating the comma and the dot alike without taking the locale > > into account may result in wrong values. > > Cheers, > > Thomas > > > > > ‐‐‐ Original Message ‐‐‐ > > > On Tuesday, May 4, 2021 6:54 AM, Albert Astals Cid aa...@kde.org wrote: > > > > > > > Added a failing test > > > > athttps://invent.kde.org/plasma-mobile/kalk/-/merge_requests/22 > > > > Now you have to make it work :) > > > > Cheers, > > > > Albert > > > > -- > > Regards > > Thomas Baumgart > > https://www.signal.org/ Signal, the better WhatsApp > > > > Of all the computing resources available, the most valuable one is > > programmers' time. Especially in open source where most of us have to > > sneak in time to write and debug code. (Ace Jones)
Re: New repo in kdereview: kalk
On Dienstag, 4. Mai 2021 19:00:13 CEST hanyoung wrote: > Flex doesn't take care of separators, MPFR and GMP do. Flex is merely > scanning for numbers and operators to pass to Bison. That's true. For lexical analysis this does not really matter. How about a space as thousand delimiter? Swedish locale: 1 000,00 Doesn't this may trick you lexer? Sorry, you asked for more of those separators/delimiters. But I think those three are the most common. Cheers Thomas > Sent with ProtonMail Secure Email. > > ‐‐‐ Original Message ‐‐‐ > On Wednesday, May 5, 2021 12:56 AM, Thomas Baumgart > wrote: > > > Han, > > > > On Dienstag, 4. Mai 2021 11:36:04 CEST hanyoung wrote: > > > > > Pushed an inelegant solution - include "," as decimal separator in flex. > > > As long as there aren't any more decimal separator we're cool. > > > > Not sure if this appropriate, but I wanted to warn you about the dilemma > > that in some locale the thousand separator and the decimal symbol are > > exactly the opposite. Example: > > > > German locale: 1.000,00 -> one thousand > > US locale : 1,000.00 -> one thousand > > > > So simply treating the comma and the dot alike without taking the locale > > into account may result in wrong values. > > > > Cheers, > > > > Thomas > > > > > ‐‐‐ Original Message ‐‐‐ > > > On Tuesday, May 4, 2021 6:54 AM, Albert Astals Cid aa...@kde.org wrote: > > > > > > > Added a failing test > > > > athttps://invent.kde.org/plasma-mobile/kalk/-/merge_requests/22 > > > > Now you have to make it work :) > > > > Cheers, > > > > Albert > > > > -- > > > > Regards > > > > Thomas Baumgart > > > > https://www.signal.org/ Signal, the better WhatsApp > > > > > > > > Of all the computing resources available, the most valuable one is > > programmers' time. Especially in open source where most of us have to > > sneak in time to write and debug code. (Ace Jones) > > > > > > > -- Regards Thomas Baumgart https://www.signal.org/ Signal, the better WhatsApp - Good judgment comes from experience, experience comes from bad judgment -- Chuck Hackett - signature.asc Description: This is a digitally signed message part.
Re: New repo in kdereview: kalk
Han, On Dienstag, 4. Mai 2021 11:36:04 CEST hanyoung wrote: > Pushed an inelegant solution - include "," as decimal separator in flex. As > long as there aren't any more decimal separator we're cool. Not sure if this appropriate, but I wanted to warn you about the dilemma that in some locale the thousand separator and the decimal symbol are exactly the opposite. Example: German locale: 1.000,00 -> one thousand US locale: 1,000.00 -> one thousand So simply treating the comma and the dot alike without taking the locale into account may result in wrong values. Cheers, Thomas > ‐‐‐ Original Message ‐‐‐ > On Tuesday, May 4, 2021 6:54 AM, Albert Astals Cid wrote: > > Added a failing test > > athttps://invent.kde.org/plasma-mobile/kalk/-/merge_requests/22 > > > > Now you have to make it work :) > > > > Cheers, > > Albert > > > -- Regards Thomas Baumgart https://www.signal.org/ Signal, the better WhatsApp - Of all the computing resources available, the most valuable one is programmers' time. Especially in open source where most of us have to sneak in time to write and debug code. (Ace Jones) - signature.asc Description: This is a digitally signed message part.
Re: New repo in kdereview: kalk
Flex doesn't take care of separators, MPFR and GMP do. Flex is merely scanning for numbers and operators to pass to Bison. Sent with ProtonMail Secure Email. ‐‐‐ Original Message ‐‐‐ On Wednesday, May 5, 2021 12:56 AM, Thomas Baumgart wrote: > Han, > > On Dienstag, 4. Mai 2021 11:36:04 CEST hanyoung wrote: > > > Pushed an inelegant solution - include "," as decimal separator in flex. As > > long as there aren't any more decimal separator we're cool. > > Not sure if this appropriate, but I wanted to warn you about the dilemma that > in some locale the thousand separator and the decimal symbol are exactly the > opposite. Example: > > German locale: 1.000,00 -> one thousand > US locale : 1,000.00 -> one thousand > > So simply treating the comma and the dot alike without taking the locale into > account may result in wrong values. > > Cheers, > > Thomas > > > ‐‐‐ Original Message ‐‐‐ > > On Tuesday, May 4, 2021 6:54 AM, Albert Astals Cid aa...@kde.org wrote: > > > > > Added a failing test > > > athttps://invent.kde.org/plasma-mobile/kalk/-/merge_requests/22 > > > Now you have to make it work :) > > > Cheers, > > > Albert > > -- > > Regards > > Thomas Baumgart > > https://www.signal.org/ Signal, the better WhatsApp > > > > Of all the computing resources available, the most valuable one is > programmers' time. Especially in open source where most of us have to > sneak in time to write and debug code. (Ace Jones) > >
Re: New repo in kdereview: kalk
Pushed an inelegant solution - include "," as decimal separator in flex. As long as there aren't any more decimal separator we're cool. Regards, Han ‐‐‐ Original Message ‐‐‐ On Tuesday, May 4, 2021 6:54 AM, Albert Astals Cid wrote: > Added a failing test > athttps://invent.kde.org/plasma-mobile/kalk/-/merge_requests/22 > > Now you have to make it work :) > > Cheers, > Albert
Re: New repo in kdereview: kalk
El dilluns, 3 de maig de 2021, a les 0:49:37 (CEST), Albert Astals Cid va escriure: > El diumenge, 2 de maig de 2021, a les 8:43:42 (CEST), hanyoung va escriure: > > Change the default output to float, however I'm not sure how to add > > autotest. I want to unit test InputManager but it's not a library. > > I wrote you an autotest > https://invent.kde.org/plasma-mobile/kalk/-/merge_requests/21 > > and while writing the autotest i realized why kalk is so broken for me. > > You don't take into account there are locales that use , as a decimal > separator instead of . > > So when i do 70/9*9 in English, it works, if i do it in Catalan it does not > because something gets confused because the result from 70/9 is not 7.77 > but 7,77 > > Sadly the autotest doesn't help catch this because i force the C locale in > it, will think how it can be improved to maybe catch that tomorrow. Added a failing test at https://invent.kde.org/plasma-mobile/kalk/-/merge_requests/22 Now you have to make it work :) Cheers, Albert > > Cheers, > Albert > > > > > ‐‐‐ Original Message ‐‐‐ > > On Sunday, May 2, 2021 6:04 AM, Albert Astals Cid wrote: > > > > > El dissabte, 1 de maig de 2021, a les 7:26:24 (CEST), hanyoung va > > > escriure: > > > > > > > I've fixed the keyboard bug and change the division result between two > > > > integers to float. > > > > > > I don't think that changing knumber to do that is a great idea, keeping > > > integer divisions as fraction instead of float seems the correct thing to > > > do, you should instead change the ui to be able to show fractions as > > > floats but to keep the internal representation as good as possible, i.e. > > > as a fraction. > > > > > > Not sure if related to that or not, but now if i type > > > 65 / 9 = * 7 = > > > > > > i get 1.3e+16 which doesn't seem correct, should be 50,... > > > > > > I'd really suggest you add autotests for this. > > > > > > Cheers, > > > Albert > > > > > > >
Re: New repo in kdereview: kalk
El diumenge, 2 de maig de 2021, a les 8:43:42 (CEST), hanyoung va escriure: > Change the default output to float, however I'm not sure how to add autotest. > I want to unit test InputManager but it's not a library. I wrote you an autotest https://invent.kde.org/plasma-mobile/kalk/-/merge_requests/21 and while writing the autotest i realized why kalk is so broken for me. You don't take into account there are locales that use , as a decimal separator instead of . So when i do 70/9*9 in English, it works, if i do it in Catalan it does not because something gets confused because the result from 70/9 is not 7.77 but 7,77 Sadly the autotest doesn't help catch this because i force the C locale in it, will think how it can be improved to maybe catch that tomorrow. Cheers, Albert > > ‐‐‐ Original Message ‐‐‐ > On Sunday, May 2, 2021 6:04 AM, Albert Astals Cid wrote: > > > El dissabte, 1 de maig de 2021, a les 7:26:24 (CEST), hanyoung va escriure: > > > > > I've fixed the keyboard bug and change the division result between two > > > integers to float. > > > > I don't think that changing knumber to do that is a great idea, keeping > > integer divisions as fraction instead of float seems the correct thing to > > do, you should instead change the ui to be able to show fractions as floats > > but to keep the internal representation as good as possible, i.e. as a > > fraction. > > > > Not sure if related to that or not, but now if i type > > 65 / 9 = * 7 = > > > > i get 1.3e+16 which doesn't seem correct, should be 50,... > > > > I'd really suggest you add autotests for this. > > > > Cheers, > > Albert >
Re: New repo in kdereview: kalk
Change the default output to float, however I'm not sure how to add autotest. I want to unit test InputManager but it's not a library. ‐‐‐ Original Message ‐‐‐ On Sunday, May 2, 2021 6:04 AM, Albert Astals Cid wrote: > El dissabte, 1 de maig de 2021, a les 7:26:24 (CEST), hanyoung va escriure: > > > I've fixed the keyboard bug and change the division result between two > > integers to float. > > I don't think that changing knumber to do that is a great idea, keeping > integer divisions as fraction instead of float seems the correct thing to do, > you should instead change the ui to be able to show fractions as floats but > to keep the internal representation as good as possible, i.e. as a fraction. > > Not sure if related to that or not, but now if i type > 65 / 9 = * 7 = > > i get 1.3e+16 which doesn't seem correct, should be 50,... > > I'd really suggest you add autotests for this. > > Cheers, > Albert
Re: New repo in kdereview: kalk
El dissabte, 1 de maig de 2021, a les 7:26:24 (CEST), hanyoung va escriure: > I've fixed the keyboard bug and change the division result between two > integers to float. I don't think that changing knumber to do that is a great idea, keeping integer divisions as fraction instead of float seems the correct thing to do, you should instead change the ui to be able to show fractions as floats but to keep the internal representation as good as possible, i.e. as a fraction. Not sure if related to that or not, but now if i type 65 / 9 = * 7 = i get 1.3e+16 which doesn't seem correct, should be 50,... I'd really suggest you add autotests for this. Cheers, Albert > Also the qml warning has been fixed. > > Regards, > Han > > ‐‐‐ Original Message ‐‐‐ > On Saturday, May 1, 2021 5:13 AM, Albert Astals Cid wrote: > > > El divendres, 30 d’abril de 2021, a les 15:31:11 (CEST), hanyoung va > > escriure: > > > > > Kalk has now switched to the (modified) KNumber as base data type now. > > > The real number precision is hard coded to 16 for now. Also during SoK, > > > binary mode was introduced. > > > > Running it i get > > > > qrc:/qml/CalculationPage.qml:85: TypeError: Property 'binaryMode' of object > > InputManager(0x55ab4db7cac0) is not a function > > > > Keyboard input seems broken (typing one 6 i get two of them to show up) > > > > Dividing doesn't work https://i.imgur.com/Zr04aki.gif > > > > Cheers, > > Albert > > > >
Re: New repo in kdereview: kalk
I've fixed the keyboard bug and change the division result between two integers to float. Also the qml warning has been fixed. Regards, Han ‐‐‐ Original Message ‐‐‐ On Saturday, May 1, 2021 5:13 AM, Albert Astals Cid wrote: > El divendres, 30 d’abril de 2021, a les 15:31:11 (CEST), hanyoung va escriure: > > > Kalk has now switched to the (modified) KNumber as base data type now. The > > real number precision is hard coded to 16 for now. Also during SoK, binary > > mode was introduced. > > Running it i get > > qrc:/qml/CalculationPage.qml:85: TypeError: Property 'binaryMode' of object > InputManager(0x55ab4db7cac0) is not a function > > Keyboard input seems broken (typing one 6 i get two of them to show up) > > Dividing doesn't work https://i.imgur.com/Zr04aki.gif > > Cheers, > Albert
Re: New repo in kdereview: kalk
El divendres, 30 d’abril de 2021, a les 15:31:11 (CEST), hanyoung va escriure: > Kalk has now switched to the (modified) KNumber as base data type now. The > real number precision is hard coded to 16 for now. Also during SoK, binary > mode was introduced. Running it i get qrc:/qml/CalculationPage.qml:85: TypeError: Property 'binaryMode' of object InputManager(0x55ab4db7cac0) is not a function Keyboard input seems broken (typing one 6 i get two of them to show up) Dividing doesn't work https://i.imgur.com/Zr04aki.gif Cheers, Albert > > Regards, > Han >
Re: New repo in kdereview: kalk
Kalk has now switched to the (modified) KNumber as base data type now. The real number precision is hard coded to 16 for now. Also during SoK, binary mode was introduced. Regards, Han
Re: New repo in kdereview: kalk
Hi all,
Cool idea for the project. I went through the C++ part of the code, and have a
few suggestions.
> > Please reconsider your decision.
> https://blog.acolyer.org/2020/10/02/toward-an-api-for-the-real-numbers/
+1 for the plea of not reinventing numerics however fun it is to play with
flex/yacc.
For the actual code review:
historymanager.cpp:
Item 1:
if (file.exists()) {
file.open(QIODevice::ReadOnly);
This should be:
if (file.open(QIODevice::ReadOnly);
The fact that a file exists does not mean open will succeed.
Item 2:
This:
for (const auto : qAsConst(array)) {
historyList.append(record.toString());
}
should be prefixed with:
historyList.reserve(array.count());
Item 3:
There is no need to call file.close() as its destructor will close it.
Item 4:
Instead of auto array and qAsConst(array), just declare array to be const:
const auto array = ...
or
const auto& array = ...
Item 5:
this->save() can be only save(). It is not common to write this-> in C++
unless there is a strong reason to do so in a particular case.
Item 6:
QList historyList; -> QList m_historyList;
inputmanager.cpp:
Item 1:
Great to see a correct C++ singleton :)
Item 2:
You can use std::deque instead of std::stack. You'll get .clear() operation.
It will be more efficient than = {};
unitmodel.cpp:
Item 1:
Similar to above, when you know the number of elements a collection will have,
call reserve(size) before you start appending (this is for m_units).
BTW, these for loops can be replaced with std::transform and
std::back_inserter, but I don't want to go overload the review. ;)
Cheers,
Ivan
--
dr Ivan Čukić
i...@cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232 07AE 01C6 CE2B FF04 1C12
Re: New repo in kdereview: kalk
On Donnerstag, 18. Februar 2021 23:55:32 CET Albert Astals Cid wrote: > El dijous, 18 de febrer de 2021, a les 17:05:22 CET, hanyoung va escriure: > > Hello everyone! > > > > I want to move kalk to kdereview. > > It's the calculator for Plasma Mobile (it also works great on desktop). > > > > https://invent.kde.org/plasma-mobile/kalk > > > > It's not a QML version of kcalc, the math engine is written with > > bison/flex. > I don't think you should roll out your own math engine. > > Example, if I press > 65 / 9 = > * 9 = > it tells me the result is 63. > > Compare to kcalc that properly tells me 65. > > You can't really use C/C++ floating point to do actual math, it's not very > good for that, you need to use "infinite" precision numbers like kcalc > does. > > Please reconsider your decision. https://blog.acolyer.org/2020/10/02/toward-an-api-for-the-real-numbers/ -- Milian Wolff m...@milianw.de http://milianw.de signature.asc Description: This is a digitally signed message part.
Re: New repo in kdereview: kalk
El dijous, 18 de febrer de 2021, a les 17:05:22 CET, hanyoung va escriure: > Hello everyone! > > I want to move kalk to kdereview. > It's the calculator for Plasma Mobile (it also works great on desktop). > > https://invent.kde.org/plasma-mobile/kalk There's a typo in the invent.kde.org description "platfrom" -> platform Cheers, Albert > > It's not a QML version of kcalc, the math engine is written with bison/flex. > And it also supports Unit conversion thanks to KUnitConversion. > > Regards, > Han >
