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 some warnings found with gcc 11 and ffmpeg master #9721

Merged
merged 7 commits into from Nov 6, 2021

Conversation

linkmauve
Copy link
Member

These are all mostly obvious.

The main remaining ones are:

  • DSP::SDSP not being pod yet offsetof() being used on it, same for memcpy() on CPMemory.
  • Some signed comparison issues where I’m not sure in which direction to cast.
  • Some invalid size copies in IOS/ES/Formats.cpp which made me notice the addition of ticket_version in Ticket which isn’t there in TicketView, making me wonder just how this ever worked.

@lioncash
Copy link
Member

offsetof on non-POD types is mostly fine, given the standard allows it to be conditionally supported by implementations (and all implementations that we care about support it).

Maybe -Wreorder should be added into the CMakeLists as an error for the codebase, given there's never a scenario where it's desirable for them to be out of order (and at worst, indicate a bug).

Source/Core/Common/CRC32.cpp Outdated Show resolved Hide resolved
Source/Core/Common/CRC32.h Outdated Show resolved Hide resolved
Source/Core/Core/IOS/USB/Bluetooth/BTReal.cpp Show resolved Hide resolved
@linkmauve linkmauve force-pushed the fix-warnings branch 2 times, most recently from 4b4a62f to c200c02 Compare May 17, 2021 20:32
@linkmauve linkmauve force-pushed the fix-warnings branch 3 times, most recently from a132d7a to 9d4685a Compare May 20, 2021 19:45
@Tilka
Copy link
Member

Tilka commented Jun 4, 2021

Please rebase and drop the -Wreorder commit as that's been fixed (sorry, I didn't check open PRs).

Source/Core/Core/ConfigLoaders/IsSettingSaveable.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Resources.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Settings/GameCubePane.cpp Outdated Show resolved Hide resolved
Source/Core/Core/Boot/Boot.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/FrameDump.cpp Show resolved Hide resolved
@linkmauve linkmauve force-pushed the fix-warnings branch 4 times, most recently from 7e6d115 to d54976f Compare October 9, 2021 21:42
@linkmauve
Copy link
Member Author

I have now fixed all of the comments you made, sorry for the huge delay in doing so…

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, 5 of 5 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @linkmauve)


Source/Core/Common/CRC32.h, line 2 at r2 (raw file):

// Copyright 2021 Dolphin Emulator Project
// Licensed under GPLv2+

These two lines should be // SPDX-License-Identifier: GPL-2.0-or-later now


Source/Core/Common/CRC32.cpp, line 2 at r2 (raw file):

// Copyright 2021 Dolphin Emulator Project
// Licensed under GPLv2+

licence header


Source/Core/Core/IOS/USB/Bluetooth/BTReal.cpp, line 551 at r1 (raw file):

    for (u8 data : entry.second)
      // We cast to u16 here in order to have it displayed as two nibbles.
      oss << std::setfill('0') << std::setw(2) << (u16)data;

static_cast<u16>(data) (also you might want to add braces since the loop body now has more than one line)


Source/Core/DolphinQt/Resources.h, line 33 at r1 (raw file):

  static QPixmap GetMisc(MiscID id);

  static QIcon GetScaledIcon(const std::string_view name);

<string_view> should be included, and the const can be removed since it doesn't do anything here.

@linkmauve
Copy link
Member Author

There, fixed all your comments.

This moves the only direct call to zlib’s crc32() into its own
translation unit, but that operation is cold enough that this won’t
matter in the slightest.  crc32_z() would be more appropriate, but
Android has an older zlib version…
Also use the correct error check for other similar calls in the same
file, despite nothing being doable on error.
This function was deprecated in ffmpeg in January[1], while its
replacement got introduced in 2015[2], so now might be the time to start
using it in Dolphin. :)

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/f7db77bd8785d1715d3e7ed7e69bd1cc991f2d07
[2] https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/a9a60106370f862e191dea58e748626da6a8fe97
Otherwise this is an error on gcc/libstdc++, and there are no transitive
includes for this header.
Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r8, 5 of 5 files at r9, 2 of 2 files at r10, 3 of 3 files at r11, 2 of 2 files at r12, 2 of 2 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @linkmauve)

@leoetlino leoetlino merged commit 2c5d11c into dolphin-emu:master Nov 6, 2021
10 checks passed
@linkmauve linkmauve deleted the fix-warnings branch November 6, 2021 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants