Skip to content

Conversation

HJfod
Copy link

@HJfod HJfod commented Feb 5, 2025

Clang on Windows defines _MSC_VER, but it doesn't seem to understand _Printf_format_string_, leading to errors code using format string parameters as it fails to understand that the parameters are format strings. However, the __attribute__((format)) syntax seems to work just fine, and so this PR enables that for Clang on Windows.

For context, I am using clang (19.1.0) instead of clang-cl (because the latter doesn't work for some unknown reasons). I've checked that this still works on MSVC, though I don't know about the rest of the platforms because I didn't realize GitHub wouldn't run CI when I made the commit. Oh well, I'll see when CI runs for the PR, and push new commits to fix it if something goes wrong.

Clang on Windows defines _MSC_VER, but it doesn't understand _Printf_format_string_
@saghul
Copy link
Contributor

saghul commented Feb 5, 2025

Do you know how we could add a Clang on windows CI target?

@HJfod
Copy link
Author

HJfod commented Feb 6, 2025

I believe just copying the action that uses clang-cl and making it use clang instead should work? Not entirely sure, can test it out on my fork

@saghul
Copy link
Contributor

saghul commented Feb 6, 2025

Please add it to this PR so we test it there too going forward.

#ifndef JS_PRINTF_FORMAT
#ifdef _MSC_VER
/* Clang on Windows doesn't seem to support _Printf_format_string_ */
#if defined(_MSC_VER) && !defined(__clang__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this change also in quickjs.h, for consistency?

#ifndef JS_PRINTF_FORMAT
#ifdef _MSC_VER
/* Clang on Windows doesn't seem to support _Printf_format_string_ */
#if defined(_MSC_VER) && !defined(__clang__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this change also in quickjs.h, for consistency?

}

#ifdef __GNUC__
#if defined(__GNUC__) || defined(__clang__)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought clang also defined GNUC, doesn't it?

Copy link
Author

Choose a reason for hiding this comment

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

At least locally that seems to not be the case when compiling on Windows, though not sure why

@HJfod
Copy link
Author

HJfod commented Feb 13, 2025

I keep running into quirks with weird things not being compatible with Clang (for example the u64 literal suffix, use of fopen etc. over fopen_s), and I don't have the time myself to work on fixing those, so this PR is most likely going to have be closed for becoming stale.

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