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 compile bug under clang(backend MSVC) #253

Closed
wants to merge 1 commit into from

Conversation

VisualGMQ
Copy link

Compile failed under:

image

Because clang's backend is MSVC, so all add_definitions apply on MSVC also should apply on clang.

Use

if(MSVC OR (CMAKE_C_COMPILER_ID STREQUAL "Clang" AND CMAKE_C_SIMULATE_ID STREQUAL "MSVC"))

to replace all MSVC detect.

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Left some comments.

endfunction()

set(is_msvc 0)
IsMSVC(is_msvc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just inline the var declaration instead of calling that parent scope-manipulating function? You're only using it once; less indirection is better, ceteris paribus.

Comment on lines 58 to +59
# ClangCL is command line compatible with MSVC, so 'MSVC' is set.
if(MSVC)
if(${is_msvc})
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment should probably be updated. Also, why write ${is_msvc} here but plain is_msvc elsewhere?

@@ -54,7 +65,7 @@ if(MSVC)
xcheck_add_c_compiler_flag(-Wno-reserved-macro-identifier)
xcheck_add_c_compiler_flag(-Wno-reserved-identifier)
xcheck_add_c_compiler_flag(-Wdeprecated-declarations)
add_compile_definitions(WIN32_LEAN_AND_MEAN)
add_compile_definitions(WIN32_LEAN_AND_MEAN _CRT_SECURE_NO_WARNINGS _CRT_NONSTDC_NO_DEPRECATE)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to keep those warnings so can you undo this change?

@@ -612,6 +612,7 @@ void rqsort(void *base, size_t nmemb, size_t size, cmp_f cmp, void *opaque)
}

#if defined(_MSC_VER)
struct timezone;
Copy link
Contributor

Choose a reason for hiding this comment

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

The include <time.h> should pull this in, right? Why doesn't that work with clang?

@@ -211,8 +211,6 @@ typedef enum {
JS_GC_PHASE_REMOVE_CYCLES,
} JSGCPhaseEnum;

typedef enum OPCodeEnum OPCodeEnum;
Copy link
Contributor

Choose a reason for hiding this comment

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

This also looks like a change that shouldn't be necessary. What exactly was the warning or error you got?

@saghul
Copy link
Contributor

saghul commented Apr 4, 2024

Ping? The CI also tests Clang 17, without failure. How exactly are you compiling it?

@saghul saghul closed this Jul 30, 2024
@saghul
Copy link
Contributor

saghul commented Jul 30, 2024

No follow-up, closing. Please holler if you want to pick this back up.

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.

None yet

3 participants