Feature/cppcheck fixes pt1 #3266

vittorioromeo · 2024-10-03T17:02:21Z

not applied:

all "Cppcheck does not need standard library headers to get proper results."
- bogus
- no side effects
all "Consider using an algorithm instead of a raw loop"
- not always better, also compile-time overhead
include/SFML/Window/Event.inl:57: warning: (error) Found an exit path from function with non-void return type that has missing return statement
- protected by static_assert
include/SFML/Graphics/Rect.inl:35: warning: (style) Function 'Rect < float >' argument 1 names different: declaration 'position' definition 'thePosition'.
- stylistical
src/SFML/Graphics/Shader.cpp:334: warning: (error) Reference to local variable returned.
- bogus
src/SFML/Graphics/Shader.cpp:322: warning: (style) 'operator=' should return reference to 'this' instance.
- bogus
src/SFML/Graphics/Texture.cpp:219: warning: (error) Reference to local variable returned.
- bogus
src/SFML/Graphics/Texture.cpp:205: warning: (style) 'operator=' should return reference to 'this' instance.
- bogus
src/SFML/System/Vector2.cpp:48: warning: (performance) Function parameter 'rhs' should be passed by const reference.
- by value is fine for vec2
src/SFML/System/Vector2.cpp:86: warning: (performance) Function parameter 'axis' should be passed by const reference.
- by value is fine for vec2

applied:

examples/X11/X11.cpp:24: warning: (style) Parameter 'window' can be declared as reference to const
examples/X11/X11.cpp:76: warning: (style) Parameter 'window' can be declared as reference to const
include/SFML/System/Utf.inl:133: warning: (style) Condition 'input<=0x0010FFFF' is always true
examples/sound_effects/SoundEffects.cpp:735: warning: (style) struct member 'Coefficients::c0' is never used.
examples/sound_effects/SoundEffects.cpp:736: warning: (style) struct member 'Coefficients::d0' is never used.
src/SFML/Audio/SoundBuffer.cpp:83: warning: (performance) Variable 'm_samples' is assigned in constructor body. Consider performing initialization in initialization list.
src/SFML/Audio/SoundBuffer.cpp:84: warning: (performance) Variable 'm_duration' is assigned in constructor body. Consider performing initialization in initialization list.
src/SFML/Audio/SoundRecorder.cpp:424: warning: (style) Condition 'channelCount==2' is always true
src/SFML/Network/SocketSelector.cpp:91: warning: (style) Parameter 'socket' can be declared as reference to const
src/SFML/Network/SocketSelector.cpp:133: warning: (style) Parameter 'socket' can be declared as reference to const
src/SFML/Network/SocketSelector.cpp:190: warning: (style) Parameter 'socket' can be declared as reference to const
src/SFML/Network/Ftp.cpp:610: warning: (style) The scope of the variable 'count' can be reduced.
src/SFML/Network/Ftp.cpp:610: warning: (style) Variable 'count' is assigned a value that is never used.
src/SFML/Main/MainAndroid.cpp:308: warning: (style) Boolean expression 'states.updated' is used in bitwise operation. Did you mean '||'?
src/SFML/Main/MainAndroid.cpp:330: warning: (style) Boolean expression 'states.updated' is used in bitwise operation. Did you mean '||'?
src/SFML/Main/MainAndroid.cpp:515: warning: (style) Boolean expression 'states->initialized' is used in bitwise operation. Did you mean '||'?
src/SFML/Window/DRM/DRMContext.cpp:521: warning: (performance) Variable 'm_display' is assigned in constructor body. Consider performing initialization in initialization list.
src/SFML/Window/DRM/DRMContext.cpp:543: warning: (performance) Variable 'm_display' is assigned in constructor body. Consider performing initialization in initialization list.
src/SFML/Window/DRM/DRMContext.cpp:563: warning: (performance) Variable 'm_display' is assigned in constructor body. Consider performing initialization in initialization list.
src/SFML/Window/DRM/DRMContext.cpp:448: warning: (style) Variable 'modeString' can be declared as pointer to const
src/SFML/Window/DRM/DRMContext.cpp:454: warning: (style) Variable 'refreshString' can be declared as pointer to const
src/SFML/Graphics/Image.cpp:426: warning: (style) Variable 'end' can be declared as pointer to const
src/SFML/Window/Unix/ClipboardImpl.cpp:146: warning: (style) Condition '!m_requestResponded' is always true
src/SFML/Window/Unix/ClipboardImpl.cpp:150: warning: (style) Condition '!m_requestResponded' is always true
src/SFML/Window/Unix/ClipboardImpl.cpp:80: warning: (performance) Variable 'm_display' is assigned in constructor body. Consider performing initialization in initialization list.
src/SFML/Window/Win32/WindowImplWin32.cpp:1130: warning: (style) Variable 'deviceBroadcastHeader' can be declared as pointer to const
src/SFML/Window/Unix/WindowImplX11.cpp:346: warning: (style) Variable 'extents' can be declared as pointer to const
src/SFML/Window/Unix/WindowImplX11.cpp:1181: warning: (style) Variable 'windowPtr' can be declared as pointer to const
src/SFML/Window/EglContext.cpp:72: warning: (style) Statements following 'return' will never be executed.
src/SFML/Window/Android/WindowImplAndroid.cpp:502: warning: (style) Function 'processPointerEvent' argument 2 names different: declaration 'event' definition 'inputEvent'.

to review:

src/SFML/Window/Unix/WindowImplX11.cpp:1721: warning: (style) Condition '!m_cursorGrabbed' is always false

There's a few more outstanding ones but I don't want to make the PR too big

eXpl0it3r · 2024-10-03T17:07:09Z

include/SFML/System/Utf.inl

+        if      (input <  0x80)     {                              bytestoWrite = 1; }
+        else if (input <  0x800)    {                              bytestoWrite = 2; }
+        else if (input <  0x10000)  {                              bytestoWrite = 3; }
+        else                        { assert(input <= 0x0010FFFF); bytestoWrite = 4; }


I assume it complains when just having an else statement with the assert?

No, the condition was always guaranteed to be true -- check the rest of the function above. The assert is something I decided to add to make that obvious to the reader.

eXpl0it3r · 2024-10-03T17:10:09Z

src/SFML/Audio/SoundRecorder.cpp

        {
+            assert(channelCount == 2);


I don't particularly like this pattern, as it move the branch condition into the assert and mixes in the body two different concerns: 1) Error checking for the input 2) actual body for the given condition

It's the same principle as above -- that condition is guaranteed to be true due to previous control flow. The assert makes that obvious to the reader.

ChrisThrasher · 2024-10-03T17:11:23Z

This PR is a mixed bag of changes. I like some and don't like others. For the sake of merging some of this stuff ASAP, I'd love all the class initializer list changes in their own PR. I'd happily approve that. We've long since made an effort to move initialization out of the constructor body and into the initializer list if not into the class definition itself. That's an easy win.

eXpl0it3r · 2024-10-03T17:14:21Z

src/SFML/Network/Ftp.cpp

@@ -620,7 +619,7 @@ void Ftp::DataChannel::send(std::istream& stream)
            break;
        }

-        count = static_cast<std::size_t>(stream.gcount());
+        const auto count = static_cast<std::size_t>(stream.gcount());

        if (count > 0)


Do we gain anything be not just inlining this?

I guess we cast the "signed" streamsize to unsigned size_t, but maybe that's not correct to begin with?

I'm not sure I get your concern -- the behavior is equivalent to before, the scope of count just used to be unnecessarily large.

I'm talking about this

Suggested change

if (count > 0)

if (stream.gcount() > 0)

or this

Suggested change

if (count > 0)

if (static_cast<std::size_t>(stream.gcount()) > 0)

vittorioromeo · 2024-10-03T17:16:47Z

This PR is a mixed bag of changes. I like some and don't like others. For the sake of merging some of this stuff ASAP, I'd love all the class initializer list changes in their own PR. I'd happily approve that. We've long since made an effort to move initialization out of the constructor body and into the initializer list if not into the class definition itself. That's an easy win.

What exactly don't you like here? I think all of these should be merged.

src/SFML/Window/Unix/ClipboardImpl.cpp

ZXShady · 2024-10-03T18:41:48Z

src/SFML/Window/Unix/WindowImplX11.cpp

@@ -343,7 +343,7 @@ bool getEWMHFrameExtents(::Display* disp, ::Window win, long& xFrameExtent, long

 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wcast-align"
-        long* extents = reinterpret_cast<long*>(data);
+        const long* extents = reinterpret_cast<long*>(data);


Suggested change

const long* extents = reinterpret_cast<long*>(data);

const auto* extents = reinterpret_cast<long*>(data);

ChrisThrasher · 2024-10-04T21:11:12Z

Rebased on master, fixed formatting, and fixed a clang-tidy failure. CI should be more green after this but I still expect the Clipboard unit tests to fail. Something about the changes to the Unix Clipboard implementation have broken it.

ChrisThrasher · 2024-10-04T22:09:06Z

src/SFML/Window/Unix/WindowImplX11.cpp

@@ -1693,6 +1693,7 @@ bool WindowImplX11::processEvent(XEvent& windowEvent)
                XSetICFocus(m_inputContext);

            // Grab cursor
+            // TODO: ???


Could you explain what these two new TODO comments mean?

I assume this is related to:

Yes, I don't know what the intended use of m_cursorGrabbed was here... Was looking for some feedback on why it was there

coveralls · 2024-10-04T23:56:09Z

Pull Request Test Coverage Report for Build 11187683706

Details

18 of 34 (52.94%) changed or added relevant lines in 10 files are covered.
9 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.005%) to 56.852%

Changes Missing Coverage	Changed/Added Lines	%
src/SFML/Audio/SoundRecorder.cpp	1	0.0%
src/SFML/Network/Ftp.cpp	1	0.0%
src/SFML/Window/Win32/WindowImplWin32.cpp	1	0.0%
src/SFML/Window/Unix/WindowImplX11.cpp	2	0.0%
src/SFML/Window/DRM/DRMContext.cpp	11	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/SFML/Audio/SoundRecorder.cpp	1	0.0%
src/SFML/Window/Unix/WindowImplX11.cpp	1	44.48%
src/SFML/Window/DRM/DRMContext.cpp	3	0.0%
src/SFML/Window/WindowImpl.cpp	4	52.29%

Totals
Change from base Build 11183712044:	0.005%
Covered Lines:	11882
Relevant Lines:	19874

💛 - Coveralls

ZXShady · 2024-10-10T10:15:16Z

src/SFML/System/Vector2.cpp:48: warning: (performance) Function parameter 'rhs' should be passed by const reference.
by value is fine for vec2

we explicitly instantiated a template for Vector2<long double> which is 32 bytes (long double is 16 bytes), sp the warning isn't bogus

vittorioromeo added the feature label Oct 3, 2024

vittorioromeo added this to the 3.0 milestone Oct 3, 2024

vittorioromeo requested review from eXpl0it3r and ChrisThrasher October 3, 2024 17:02

eXpl0it3r reviewed Oct 3, 2024

View reviewed changes

src/SFML/Window/Unix/ClipboardImpl.cpp Outdated Show resolved Hide resolved

ZXShady reviewed Oct 3, 2024

View reviewed changes

ChrisThrasher force-pushed the feature/cppcheck_fixes_pt1 branch from f59a364 to 6100b40 Compare October 4, 2024 21:10

ChrisThrasher force-pushed the feature/cppcheck_fixes_pt1 branch from 6100b40 to 4920c0b Compare October 4, 2024 22:02

ChrisThrasher reviewed Oct 4, 2024

View reviewed changes

Cppcheck fixes part 1

002cd1d

ChrisThrasher force-pushed the feature/cppcheck_fixes_pt1 branch from 4920c0b to 002cd1d Compare October 4, 2024 22:13

eXpl0it3r modified the milestones: 3.0, 3.1 Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/cppcheck fixes pt1 #3266

Feature/cppcheck fixes pt1 #3266

vittorioromeo commented Oct 3, 2024

eXpl0it3r Oct 3, 2024

vittorioromeo Oct 3, 2024

eXpl0it3r Oct 3, 2024

vittorioromeo Oct 3, 2024

ChrisThrasher commented Oct 3, 2024 •

edited

Loading

eXpl0it3r Oct 3, 2024

vittorioromeo Oct 3, 2024

eXpl0it3r Oct 3, 2024

vittorioromeo commented Oct 3, 2024

ZXShady Oct 3, 2024

ChrisThrasher commented Oct 4, 2024

ChrisThrasher Oct 4, 2024

eXpl0it3r Oct 4, 2024

vittorioromeo Oct 5, 2024

coveralls commented Oct 4, 2024

ZXShady commented Oct 10, 2024 •

edited

Loading

	if (count > 0)
	if (static_cast<std::size_t>(stream.gcount()) > 0)

	const long* extents = reinterpret_cast<long*>(data);
	const auto* extents = reinterpret_cast<long*>(data);

Feature/cppcheck fixes pt1 #3266

Are you sure you want to change the base?

Feature/cppcheck fixes pt1 #3266

Conversation

vittorioromeo commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisThrasher commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vittorioromeo commented Oct 3, 2024

Choose a reason for hiding this comment

ChrisThrasher commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 4, 2024

Pull Request Test Coverage Report for Build 11187683706

Details

💛 - Coveralls

ZXShady commented Oct 10, 2024 • edited Loading

ChrisThrasher commented Oct 3, 2024 •

edited

Loading

ZXShady commented Oct 10, 2024 •

edited

Loading