Skip to content
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

Fix warnings and android issues #2245

Merged
merged 6 commits into from
Sep 12, 2024

Conversation

DreamNik
Copy link
Contributor

@DreamNik DreamNik commented Sep 5, 2024

Follow up and fix for #2243

@@ -21,12 +21,13 @@
#include "channelpowersink.h"

ChannelPowerSink::ChannelPowerSink(ChannelPower *channelPower) :
m_channelPower(channelPower),
Copy link
Owner

@f4exb f4exb Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be removed from the constructor parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -811,6 +811,10 @@ class ModelMatch {
{
m_aircraftRegExp.optimize();
}

virtual ~ModelMatch()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or "virtual ~ModelMatch = default" or better "~ModelMatch final = default"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, default semantics is better :)
I'll keep virtual destructor, because class isn't final, so finalizing it's destruction less consistent.

Copy link
Contributor Author

@DreamNik DreamNik Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, there are lot of warnings like "warning: class with destructor marked 'final' cannot be inherited from [-Wfinal-dtor-non-final-class]". So marking destructor final causes more warnings.

@@ -24,7 +24,6 @@
#include "aptdemodsink.h"

APTDemodSink::APTDemodSink(APTDemod *packetDemod) :
m_aptDemod(packetDemod),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as with channelpowersink.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -488,7 +488,6 @@ void DABDemodSink::processOneAudioSample(Complex &ci)
}

DABDemodSink::DABDemodSink(DABDemod *packetDemod) :
m_dabDemod(packetDemod),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -31,7 +31,6 @@
ILSDemodSink::ILSDemodSink(ILSDemod *ilsDemod) :
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -28,7 +28,6 @@
#include "navtexdemodsink.h"

NavtexDemodSink::NavtexDemodSink(NavtexDemod *packetDemod) :
m_navtexDemod(packetDemod),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -30,7 +30,6 @@

PagerDemodSink::PagerDemodSink(PagerDemod *pagerDemod) :
m_scopeSink(nullptr),
m_pagerDemod(pagerDemod),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -29,7 +29,6 @@
#include "rttydemodsink.h"

RttyDemodSink::RttyDemodSink(RttyDemod *packetDemod) :
m_rttyDemod(packetDemod),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -27,7 +27,6 @@
#include "freqscannersink.h"

FreqScannerSink::FreqScannerSink(FreqScanner *ilsDemod) :
m_freqScanner(ilsDemod),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -26,7 +26,6 @@

HeatMapSink::HeatMapSink(HeatMap *heatMap) :
m_scopeSink(nullptr),
m_heatMap(heatMap),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -42,7 +42,7 @@ struct LocalSinkSettings
uint32_t m_log2FFT;
FFTWindow::Function m_fftWindow;
bool m_reverseFilter;
static const uint32_t m_maxFFTBands = 20;
uint32_t m_maxFFTBands;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not keep it as static const ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As is it throws error that m_maxFFTBands couldn't be resolved.
Because "static const" member isn't definition but rather a declaration.

@@ -53,6 +53,7 @@ void LocalSinkSettings::resetToDefaults()
m_reverseAPIChannelIndex = 0;
m_workspaceIndex = 0;
m_hidden = false;
m_maxFFTBands = 32;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 32 and not leave it at 20?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, leftovers from experiments :) I'll undo the change.

@@ -26,7 +26,6 @@
#include "noisefiguresink.h"

NoiseFigureSink::NoiseFigureSink(NoiseFigure *noiseFigure) :
m_noiseFigure(noiseFigure),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -28,7 +28,6 @@

RadioClockSink::RadioClockSink(RadioClock *radioClock) :
m_scopeSink(nullptr),
m_radioClock(radioClock),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from constructor parameters if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -54,8 +54,7 @@ FileSource::FileSource(DeviceAPI *deviceAPI) :
ChannelAPI(m_channelIdURI, ChannelAPI::StreamSingleSource),
m_deviceAPI(deviceAPI),
m_frequencyOffset(0),
m_basebandSampleRate(0),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing it from the initialization list ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"m_linearGain" is never used.
And the line with 'm_basebandSampleRate' only got comma removed.

@@ -273,8 +273,6 @@ int DVB2::next_ts_frame_base( u8 *ts )
}
DVB2::DVB2(void)
{
// Clear the transport queue
m_tp_q.empty();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_tp_q appears to be unused so it can be removed from the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in "unpack_transport_packet_add_crc" as a temporary storage. But I think it's kept inside class to (maybe?) reuse some internal structures.

sampleMic = (int)((signed char) buffer[b++]) << 8;
sampleMic += (int)((unsigned char)buffer[b++]);
// sampleMic
b+=2;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it !

@@ -66,7 +66,7 @@ uint32_t RTPRandom::PickSeed()
#else
x += (uint32_t)clock();
#endif
x ^= (uint32_t)((uint8_t *)this - (uint8_t *)0);
x ^= (uint32_t)(size_t)this;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is substantially different from the original. Did you test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"rtprandom.cpp:69:37: Performing pointer subtraction with a null pointer may have undefined behavior"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, for C this is definitely UB. For C++ this should be valid conversion. To make compiler happy i've changed it.
Since this is just a random number generator and converting pointer to integer is valid operation, should be ok.

@@ -18,7 +18,13 @@

#include <QOpenGLShaderProgram>
#include <QOpenGLFunctions>
#include <QOpenGLFunctions_3_3_Core>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not in the #else next? Did you test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix that. Don't know why, maybe copy-paste from glshadertvarray.h.
Shouldn't be an issue since none of the QOpenGLFunctions_*_Core headers are used. Probably safe to remove this include completely. But I can't test for all platforms, so I'll keep the include.

@@ -3065,7 +3065,7 @@ void MainWindow::keyPressEvent(QKeyEvent* event)
}
}

void MainWindow::orientationChanged(Qt::ScreenOrientation orientation) const
void MainWindow::orientationChanged(Qt::ScreenOrientation orientation)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any valid reason to remove const ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, "setTabPosition" ,ethod is non-const, so calling it from const method isn't correct.

@@ -229,7 +229,7 @@ private slots:
void openDeviceSetPresetsDialog(QPoint p, const DeviceGUI *deviceGUI);
void commandKeyPressed(Qt::Key key, Qt::KeyboardModifiers keyModifiers, bool release) const;
void fftWisdomProcessFinished(int exitCode, QProcess::ExitStatus exitStatus);
void orientationChanged(Qt::ScreenOrientation orientation) const;
void orientationChanged(Qt::ScreenOrientation orientation);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any valid reason to remove const ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, "setTabPosition" ,ethod is non-const, so calling it from const method isn't correct.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a code smell pointed out by Sonarlint so I am surprised this is not actually const. We'll sort this out later...

Copy link

sonarqubecloud bot commented Sep 9, 2024

@f4exb f4exb merged commit d6773ea into f4exb:master Sep 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants