New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checking with PVS-Studio static analyser. #3054

Merged
merged 3 commits into from Apr 29, 2017

Conversation

Projects
None yet
3 participants
@SvyatoslavRazmyslov
Contributor

SvyatoslavRazmyslov commented Apr 28, 2017

We have found and fixed a bug using PVS-Studio tool. PVS-Studio is a static code analyzer for C, C++ and C#: https://www.viva64.com/en/pvs-studio/

We suggests having a look at the emails, sent from @pvs-studio.com.

Analyzer warnings:

  • V519 The 'uri' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 96, 97. RichTextEditor.cpp 97
  • V519 The 'needclose' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 542, 544. RichTextEditor.cpp 544
  • V547 Expression 'aos' is always true. AudioOutput.cpp 506
  • V547 Expression '!dlg' is always true. MainWindow.cpp 2380
  • V612 An unconditional 'return' within a loop. Database.cpp 224
  • V612 An unconditional 'return' within a loop. Database.cpp 247
  • V612 An unconditional 'return' within a loop. Database.cpp 293
@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 28, 2017

Member

Good day and thank you for your effort and providing the results.

The first email looked completely automated, so I had little interest in replying. The second one, I did reply, but your email server does not accept secure TLS connections, in which case my mail provider stops delivery by default (as per my setting). Maybe you can poke someone about that? I have not come around to temporarily disable this setting in order to resend the reply to you. 😃

I had also run PVS-Studio myself, and fixed some stuff #3051, but the ones you listed did not show IIRC.

Member

Kissaki commented Apr 28, 2017

Good day and thank you for your effort and providing the results.

The first email looked completely automated, so I had little interest in replying. The second one, I did reply, but your email server does not accept secure TLS connections, in which case my mail provider stops delivery by default (as per my setting). Maybe you can poke someone about that? I have not come around to temporarily disable this setting in order to resend the reply to you. 😃

I had also run PVS-Studio myself, and fixed some stuff #3051, but the ones you listed did not show IIRC.

@Kissaki Kissaki requested a review from mkrautz Apr 28, 2017

Show outdated Hide outdated src/mumble/AudioOutput.cpp
@@ -496,19 +496,15 @@ bool AudioOutput::mix(void *outbuff, unsigned int nsamp) {
if (recorder) {
AudioOutputSpeech *aos = qobject_cast<AudioOutputSpeech *>(aop);
Q_ASSERT(aos != nullptr);

This comment has been minimized.

@mkrautz

mkrautz Apr 29, 2017

Member

We can't use nullptr here. Mumble 1.3.0 should still build in C++98/C++03 mode.

@mkrautz

mkrautz Apr 29, 2017

Member

We can't use nullptr here. Mumble 1.3.0 should still build in C++98/C++03 mode.

This comment has been minimized.

@mkrautz

mkrautz Apr 29, 2017

Member

Also, since this is a qobject_cast, aos being null is not a bug. We should just remove the assert.

@mkrautz

mkrautz Apr 29, 2017

Member

Also, since this is a qobject_cast, aos being null is not a bug. We should just remove the assert.

This comment has been minimized.

@Kissaki

Kissaki Apr 29, 2017

Member

Oh shit, how did I even miss this? I briefly looked at this wondering, but then went through the list in the description.

It seems I only looked at the change below, which is fine even without the assertion (dropping an additional if that is always true after the first one). I totally missed the assert then.

Yeah, removing the assert.

@Kissaki

Kissaki Apr 29, 2017

Member

Oh shit, how did I even miss this? I briefly looked at this wondering, but then went through the list in the description.

It seems I only looked at the change below, which is fine even without the assertion (dropping an additional if that is always true after the first one). I totally missed the assert then.

Yeah, removing the assert.

@mkrautz

The Q_ASSERT after qobject_cast is not correct. Other than that, looks good.

@Kissaki I won't have time to immediately do this change, so feel free... :-)

@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 29, 2017

Member

I already removed it via Github edit. :)

Member

Kissaki commented Apr 29, 2017

I already removed it via Github edit. :)

@Kissaki Kissaki merged commit 5a5a3b2 into mumble-voip:master Apr 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Kissaki

This comment has been minimized.

Show comment
Hide comment
@Kissaki

Kissaki Apr 29, 2017

Member

Thank you for your contribution!

Member

Kissaki commented Apr 29, 2017

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment