Counting Bugs in Windows Calculator

[mastax]

> The analyzer has detected two identically implemented functions. As their names, Convert and ConvertBack, suggest, they were meant to do different things, but the developers should know better.

This is not a bug. Look how the `BooleanNegationConverter` is used:

    <RadioButton x:Name="fullKeypad"
     ...
     IsChecked="{x:Bind Model.IsBitFlipChecked, Converter={StaticResource BooleanNegationConverter}, Mode=TwoWay}"/>

    <RadioButton x:Name="bitFlip"
     ...
     IsChecked="{x:Bind Model.IsBitFlipChecked, Mode=TwoWay}"/>

Two radio buttons bound to the same boolean value, one with the negation converter. If `fullKeypad` is checked, `IsBitFlipChecked` should be set to false. If `IsBitFlipChecked` is set to true, `fullKeypad.IsChecked` should be set to false. The converter needs to do the same operation in both directions.

[taco_emoji]

Agree it's not a bug as I assume it's an implementation of the IValueConverter interface: https://docs.microsoft.com/en-us/dotnet/api/system.windows.d...

However since the method bodies are identical, IMO one should have the full implementation and then the other should just call the first. Still not a bug, just a code smell.

[geofft]

Is this PVS-Studio blogspam? They're always so proud of being able to detect the same things gcc/clang -Wall -Wextra detects.

[detaro]

seems so: https://www.viva64.com/en/b/0615/

[AndreyKarpov]

PVS-Studio vs Compilers:

Clang: https://www.viva64.com/en/b/0108/ , https://www.viva64.com/en/b/0155/ , https://www.viva64.com/en/b/0446/

GCC: https://www.viva64.com/en/b/0425/

[sonofgod]

In 'Suspicious comparison of real numbers':

isActive = ((ratio > threshold) || (ActiveIfEqual && (ratio == threshold)));

is called out as bad code by the blogger; this feels very much like a false positive, given that we're checking an inequality already; just effectively changing it from > to >=

A good example of not just blindly following the linter...

[lifthrasiir]

Yet another example of "suspicious" test code which intention is very clear:

    TEST_METHOD(TestSwitchAndReselectCurrentlyActiveValueDoesNothing)
    {
      // *snip*
      vm.Value2Active = true;
      // Establish base condition
      VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount);
      vm.Value2Active = true;
      VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount);
    }

> [...] The analyzer has detected two identical code fragments executing immediately one after the other. It looks like this code was written using the copy-paste technique and the programmer forgot to modify the copies.

Here Value2Active is a C++/CX property rigged to a PropertyChanged event [1] with a macro:

    public ref class UnitConverterViewModel sealed: public Windows::UI::Xaml::Data::INotifyPropertyChanged
    {
        // other members omitted
        OBSERVABLE_OBJECT();
        OBSERVABLE_PROPERTY_RW(bool, Value2Active);
    };

Can you be sure that this actually does not have a side effect? I bet not. Indeed, the intention of the original test is clear: a property, once set ("Switch") to a particular value, should not update other states even when set ("Reselect") to that same value---as it will really trigger a side effect! This idempotency guarantee is an important interface contract worth testing, no matter what a linter and clueless blog author says.

[1] https://github.com/Microsoft/calculator/blob/master/docs/App...

[bloomer]

Yeah that whole section on suspicious comparison looks like false positives by someone who didn't even bother to look at the code or doesn't understand it. Right after there is also the flagging of strod(rounded string) = 0.0 which I'm pretty sure is correct. They are checking whether the rounded string is exactly zero. Automated tooling is really only useful if you actually know what you are doing and can properly triage the output. "Look I ran this tool and it spit out some things" isn't very helpful.

[scscsc]

Still, the comparison ratio == threshold has very little chance to turn out true.

When you want to check whether A >= B (where A and B are numbers represented as floats), you would probably want to use the check A >= B - epsilon.

[gitgud]

It's interesting to see the transition from the old Proprietary Microsoft, to the new Open-Source Microsoft. Opening up code like this, which is unpolished and has bugs, is a sign of good will and transparency which I appreciate.

Although considering this application Calculator could be one of the most-used programs on Windows, and they left that many errors in. It's scary to think how many errors could be in other parts of the system...

[mschaef]

> and they left that many errors in.

How many of those errors are visible to end users in a way that actually matters? A 'suspicious' floating point conversion in a display aspect ratio check? A memory leak in a program that's almost entirely interactively driven? I don't see anything else terribly huge on the list either.

Software errors have an impact to the user and a cost to fix. These don't seem like errors where the ROI on the fix was likely to be positive, so maybe it's reasonable they were left in.

You should also consider the errors that Microsoft _removed_ from the calculator before passing too much judgement on their software development process. After all, this is the same company that replaced IEEE754 floating point in calculator with an arbitrary precision library. (At considerably more expense than any of the bug fixes indicaetd by the attached article)

https://blogs.msdn.microsoft.com/oldnewthing/20040525-00/?p=...

[saagarjha]

> considering this application Calculator could be one of the most-used programs on Windows

I'd seriously doubt that. I'm sure browsers and word processors are used far more often, and that's not even including programs that run every time the computer boots!

[anticodon]

BTW, I think that python/ipython cli is a much better calculator. I stopped using other calculators long time ago. Although I understand that it's probably too complicated for ordinary user.

[nerdbaggy]

I think all the code errors like this is why some people are scared to open source their code. While it’s a great learning experience some people just get overwhelmed with the possibility of wrongness so they don’t.

[uranusjr]

I’d go as far as saying that this kind of posts worsen the situation. In general, if you find bugs in open source code, it’s better to quietly file bugs to the author instead of writing a full post listing errors people made. It’s like your high school teacher calling you out in front of class for mistakes you made in a test. Not cool.

[zamalek]

> Suspicious function sequence

As far as I remember, and it has been a while (and is virtually undocumented), with ETW the convention is to not call the EndX method when an error occurs as EndX is basically synonymous with `return`. This might have been carried over.

Either way, I'm sure there are false positives, as always with static analysis.

[palotasb]

This seems like a stolen article. The URL should be changed to the original blog post: https://www.viva64.com/en/b/0615/

[golergka]

They usually repost their articles on Habr themselves in their corporate blog, linking to the source. It's a very popular Russian IT resource, so they do it to promote their articles, nothing is stolen, it's just reposted.

[paulriddle]

It is the same author - SvyatoslavMC on habr.com and Svyatoslav Razmyslov on viva64.com. So it isn't a problem.

[Latteland]

Thanks for the pointer, that was actually interesting. No one looks at most old code bases unless they are changing something in particular, that's how old bugs hang around forever. Commercial software companies of the world, open source your old crappy utilities and the world may improve them. Of course you don't do that because you are embarassed at their poor quality. Based on my own personal experience :-). You should have seen the source code of sybase sql server.

[Narishma]

This isn't an old code base though. It's the new UWP calculator that was introduced in Windows 10 (or was it 8?).

[showdead]

Calculator is interesting in that there is a physical button on (many) keyboards to open it. Ironically enough, I encountered a bug with the Windows 10 calculator that caused it to crash upon opening: the window would draw, then a second later vanish.

Turned out that it was due to something profile related: re-registering the Windows App Store fixed the issue. Yes, that's right, a dependency on the app store can prevent you from running Calculator.

[julienevermind]

There's another article on calculators' code check. This time is Qalculate - a multi-purpose cross-platform desktop calculator. https://habr.com/en/company/pvs-studio/blog/443656/

[tonyedgecombe]

It’s interesting that most of these would have been avoided by using a safer language than C++.

[renholder]

Calculator has been around since ninteen-eighty-something-or-other[0] and this seems to be a revamp of the base code, with a XAML (read: WPF) wrapper[1].

Aside from the obvious answer of them using C++ based upon "using something you know and that works", why shouldn't they have used C++? (Genuinely wanting to know/understand.)

It would seem like a monolithic task to rearchitecture the entire app versus just dropping the code-behind into a new UI wrapper, yeah? I'm not excusing it, to be sure, just explaining the potential ROI perspective they might've had when they stuck with C++.

[0] - https://en.wikipedia.org/wiki/Windows_Calculator#History

[1] - https://en.wikipedia.org/wiki/Windows_Calculator#Windows_10

[EpicBlackCrayon]

Yeah, nobody is talking about this but I think you're right on the money.

[kriive]

Rust Evangelism Strike Force

[matmg]

Rust will always be niche. It won't win to C/C++. Sorry mate.