X Tutup
Skip to content

BACKENDS: MACOS: Fix various small warnings in macosx-audiocd.cpp#6477

Merged
dwatteau merged 2 commits intoscummvm:masterfrom
dwatteau:fix/macosx-audiocd-warning-fixes
Apr 16, 2025
Merged

BACKENDS: MACOS: Fix various small warnings in macosx-audiocd.cpp#6477
dwatteau merged 2 commits intoscummvm:masterfrom
dwatteau:fix/macosx-audiocd-warning-fixes

Conversation

@dwatteau
Copy link
Contributor

@dwatteau dwatteau commented Mar 10, 2025

This is for macosx-audiocd.cpp (backend code).

  • Fix a -Wframe-larger-than warning (macOS 13.7.4, Intel x64, Apple Clang 14), by allocating the statfs buffer on the heap instead of the stack
  • Fix a -Wsign-compare warning when comparing with UINT_MAX (macOS 10.4.11, ppc32, GCC 7.5.0), with an unsigned long cast (since we already check trackID > 0 just before it)

While there:

  • Add a couple more headers for getfsstat(), as given by its manual page (although it always built just fine without them).

I can split this into multiple commits if you'd like, but I'm not sure it's worth it here.

Tested on macOS 10.4 (ppc32), 13.7 (Intel x64) and 15.3 (aarch64).

@criezy: Does this look good to you? Thanks :)

Allocate the `struct statfs` buffer on the heap instead of the stack,
fixing a -Wframe-larger-than warning.

Fix a -Wsign-compare warning with the GCC 32-bit builds.

Add a couple more headers for getfsstat(), as given by its manual page
(although it builds just fine without them).
Copy link
Member

@criezy criezy left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@dwatteau dwatteau marked this pull request as draft March 11, 2025 22:24
…ant buffer

For the statfs buffer used by detectAllDrives(), only allocate what is
needed, instead of an arbitrary value.

Calling getfsstat() a first time with a NULL pointer will return the
number of mounted file systems. We can then use that a allocate a
buffer at the right size for the next getfsstat() call.

It looks like we can then reasonably expect MNT_NOWAIT to be enough
for the second call that immediately follows...

Idea from criezy.
@dwatteau dwatteau force-pushed the fix/macosx-audiocd-warning-fixes branch from 88d82b6 to 74b34a2 Compare March 18, 2025 20:36
@dwatteau dwatteau marked this pull request as ready for review March 18, 2025 20:45
@dwatteau
Copy link
Contributor Author

dwatteau commented Mar 18, 2025

@criezy I've made some adjustments from your last review.

FWIW, alternatives to getfsstat() may be:

  • getmntinfo(3), but it's not thread-safe
  • NSFileManager has some mountedVolume stuff, as a higher-level API.
  • DiskArbitration may have some higher-level stuff as well.

the last two APIs maybe suit this code better. Maybe they'd make detecting an audio CD drive faster, too, but I haven't tested that.

I don't think we'd gain much by switching to some other API anyway. The audiocd code is probably less and less used today, so I'd rather not diverge too much from something that didn't cause much bug reports in the 12 years it's been there.

OK for you? Thanks!

@dwatteau
Copy link
Contributor Author

Merging, since there was no objection. Thanks!

@dwatteau dwatteau merged commit 39e24ca into scummvm:master Apr 16, 2025
8 checks passed
@dwatteau dwatteau deleted the fix/macosx-audiocd-warning-fixes branch April 16, 2025 14:11
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

X Tutup