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 UWP build #611

Closed
wants to merge 1 commit into from
Closed

fix UWP build #611

wants to merge 1 commit into from

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented May 23, 2023

BY_HANDLE_FILE_INFORMATION is not defined when WINSTORECOMPAT is not defined (in mingw-w64 and never in MSVC).

follow up to 7dfcd73

BY_HANDLE_FILE_INFORMATION is not defined when WINSTORECOMPAT is not set
(in mingw-w64 and never in MSVC).

follow up to 7dfcd73
@sezero
Copy link
Contributor

sezero commented May 23, 2023

With plain mingw build, I get:

  CC       grabbag/file.lo
../../../src/share/grabbag/file.c: In function 'grabbag__file_are_same':
../../../src/share/grabbag/file.c:147:2: warning: ISO C90 forbids mixed declarations and code
  CCLD     grabbag/libgrabbag.la

Maybe enclose the info1|2 stuff in braces to avoid that?

@sezero
Copy link
Contributor

sezero commented May 23, 2023

Also, there seems to be an else missing in there because the info1|2 stuff are supposed to work only on valid handles.

I suggest something like the following, on top of your patch. (formatting excluded deliberately.)

diff --git a/src/share/grabbag/file.c b/src/share/grabbag/file.c
index 652ef70..d20f9b8 100644
--- a/src/share/grabbag/file.c
+++ b/src/share/grabbag/file.c
@@ -143,6 +143,8 @@ FLAC__bool grabbag__file_are_same(const char *f1, const char *f2)
 	h2 = CreateFile_utf8(f2, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
 	if(h1 == INVALID_HANDLE_VALUE || h2 == INVALID_HANDLE_VALUE)
 		ok = 0;
+	else
+	{
 #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP)
 	BY_HANDLE_FILE_INFORMATION info1, info2;
 	ok &= GetFileInformationByHandle(h1, &info1);
@@ -159,7 +161,9 @@ FLAC__bool grabbag__file_are_same(const char *f1, const char *f2)
 	       GetFileInformationByHandleEx(h2, FileIdInfo, &id_info2, sizeof (id_info2)) &&
 	       id_info1.VolumeSerialNumber == id_info2.VolumeSerialNumber &&
 	       memcmp(&id_info1.FileId, &id_info2.FileId, sizeof(id_info1.FileId)) == 0;
+
 #endif // !WINAPI_PARTITION_DESKTOP
+	}
 	if(h1 != INVALID_HANDLE_VALUE)
 		CloseHandle(h1);
 	if(h2 != INVALID_HANDLE_VALUE)

@robUx4
Copy link
Contributor Author

robUx4 commented May 24, 2023

With plain mingw build, I get:

  CC       grabbag/file.lo
../../../src/share/grabbag/file.c: In function 'grabbag__file_are_same':
../../../src/share/grabbag/file.c:147:2: warning: ISO C90 forbids mixed declarations and code
  CCLD     grabbag/libgrabbag.la

Maybe enclose the info1|2 stuff in braces to avoid that?

Sure it's possible, but it's just a warning. Any modern C compiler (including that one) understands this. I can completely refactor the code to support C89 or K&R C, but is that really supposed to be supported when targeting Windows ? I can understand it may be needed for some embedded platform, but I doubt it on Windows. Is there a minimum requirement for the C standard supported ? There doesn't seem to be any set in the autoconf or CMake files.

The configure.ac says this:

#Prefer whatever the current ISO standard is.
AC_PROG_CC

@robUx4
Copy link
Contributor Author

robUx4 commented May 24, 2023

Also, there seems to be an else missing in there because the info1|2 stuff are supposed to work only on valid handles.

What makes you think that ? The documentation doesn't say that:

And this code executes just fine and return FALSE on both case:

    BY_HANDLE_FILE_INFORMATION old;
    BOOL old_ret = GetFileInformationByHandle(INVALID_HANDLE_VALUE, &old);
    FILE_BASIC_INFO new;
    BOOL new_ret = GetFileInformationByHandleEx(INVALID_HANDLE_VALUE, FileBasicInfo, &new, sizeof(new));

Also this is out of the scope of what this MR is fixing.

@robUx4
Copy link
Contributor Author

robUx4 commented May 24, 2023

Closing this as it's an issue in mingw-w64 and not really a FLAC issue (see #612).

@robUx4 robUx4 closed this May 24, 2023
@robUx4
Copy link
Contributor Author

robUx4 commented May 24, 2023

BTW this line in a public header says it assumes that C99 headers are present:

/* This of course assumes C99 headers */

It would be a good idea to set that minimum in the CMake and autotools projects.

@ktmf01
Copy link
Collaborator

ktmf01 commented May 24, 2023

Here is some discussion about supporting C89 with some C99 headers: #483 I'd like to keep the assumptions about C standard version as compatible as possible (within reason) as libFLAC is used in quite a few embedded applications as well. In that case, that means C89, with the exception of some headers. stdint.h and stdbool.h shouldn't be too hard to "emulate".

FWIW I also think it is good practice (or at least part of the coding style) to not mix declarations and code.

@robUx4
Copy link
Contributor Author

robUx4 commented May 24, 2023

If you want to make sure this compatibility doesn't break, you should enforce it in CMake with set(CMAKE_C_STANDARD 90). Or at least in the CI with -DCMAKE_C_STANDARD=90.

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.

3 participants