Fix compiler warnings with MacOS Clang 14.0.3 #6467

michael-wb · 2023-04-04T06:21:40Z

What, How & Why?

MacOS Clang 14.0.3 enables -Wunqualified-std-cast-call and -Wunused-but-set-variable warnings. Fix the unqualified move's and existing unused-but-set-variable warnings in the Realm Core source. The warning is disabled only for the bison and flex generated parser files and the Catch2 library.

Updated

☑️ ToDos

📝 Changelog update
~~[ ] 🚦 Tests (or not relevant)~~
~~[ ] C-API, if public C++ API changed.~~

kiburtse · 2023-04-04T13:21:52Z

Why introduce this project wide? It's pretty useful, why not to fix warnings? There are not that many of them

michael-wb · 2023-04-04T13:36:58Z

Why introduce this project wide? It's pretty useful, why not to fix warnings? There are not that many of them

There are some of these warnings in the 3rd party catch and generated/parser source as well - I can fix the couple of warnings in the regular source.

kiburtse · 2023-04-04T14:00:57Z

Why introduce this project wide? It's pretty useful, why not to fix warnings? There are not that many of them

There are some of these warnings in the 3rd party catch and generated/parser source as well - I can fix the couple of warnings in the regular source.

Well it's fine to set flags for external dependencies, but would be good to keep our codebase clean at least.

tgoyne · 2023-04-04T20:38:24Z

CMakeLists.txt

@@ -57,6 +57,17 @@ function(add_cxx_flag_if_supported flag)
    endif()
 endfunction()

+function(check_if_cxx_compiler_flag_supported flag out_var)
+    if(flag MATCHES "^-Wno-")
+        # Compilers ignore unknown -Wno-foo flags, so look for -Wfoo instead.


If we're assuming unrecognized -Wno flags will be ignored can't we just set -Wno-unused-but-set-variable unconditionally?

I had originally just disabled it for the project, but I got some push back, so I disabled it just for the 3rd party items. I guess it is a valid warning, similar to the -Wunused-variable warning that is already enabled. There were only 2 places where this warning was found in core, but they were also found in Catch2 and the flex generated parser cpp file.

Oh - I misread your comment. I think this was there to avoid a false positive to determine if a no- flag was really supported or not by the compiler.

If passing -Wno to the compiler doesn't result in an error we don't really care if it's actually doing anything or not, so doing complicated things to try to check if it's actually disabling a warning seems pointless.

I see your point and I can change it if you prefer, but I copied this function wholesale from this commit: ca39b9f and it also matches the existing add_cxx_flag_if_supported function already in the same CMakeLists.txt file.

IIRC unrecognized -Wno-blah flags aren't exactly ignored. There are in a weird Schrödinger state where they act as-if ignored unless the compiler prints anything else, in which case it also prints a warning about an unrecognized flag. This can be annoying if you pass tons of -Wno-blah flags unconditionally. Note that we are already doing something like this:

realm-core/CMakeLists.txt

Lines 46 to 58 in 063927d

function(add_cxx_flag_if_supported flag)

if(flag MATCHES "^-Wno-")

# Compilers ignore unknown -Wno-foo flags, so look for -Wfoo instead.

string(REPLACE "-Wno-" "-W" check_flag ${flag})

else()

set(check_flag ${flag})

endif()

check_cxx_compiler_flag(${check_flag} HAVE${check_flag})

if(HAVE${check_flag})

add_compile_options($<$<COMPILE_LANGUAGE:CXX>:${flag}>)

endif()

endfunction()

I added that logic based on this code from the mongo build scripts.

We should probably dedupe this logic between the functions though.

Ah right, I remember running into that before.

RedBeard0531

Please change add_cxx_flag_if_supported to use check_if_cxx_flag_is_supported rather than duplicating the logic.

RedBeard0531 · 2023-04-05T09:44:52Z

CMakeLists.txt

@@ -112,6 +123,8 @@ else()
    # compilers that we still support. It is also harmless, unlike pessimizing moves.
    add_cxx_flag_if_supported(-Wno-redundant-move)

+    check_if_cxx_compiler_flag_supported(-Wno-unused-but-set-variable HAS-Wno-unused-but-set-variable)


Since the output is only used in one place, I think I'd move this there.

…tion

michael-wb · 2023-04-05T15:55:32Z

FYI - The Jenkins failure is due to the results file not being exported successfully from the iOSSimulator, even though all the tests passed. This happens occasionally and we have an existing task to look into this.

CMakeLists.txt

src/realm/parser/CMakeLists.txt

RedBeard0531 · 2023-04-05T16:39:46Z

CMakeLists.txt

-        # Compilers ignore unknown -Wno-foo flags, so look for -Wfoo instead.
-        string(REPLACE "-Wno-" "-W" check_flag ${flag})
+function(append_source_file_compile_options file property)
+    get_source_file_property(SRC_PROPS_${file} ${file} COMPILE_OPTIONS)


Suggested change

get_source_file_property(SRC_PROPS_${file} ${file} COMPILE_OPTIONS)

get_source_file_property(OLD_OPTS ${file} COMPILE_OPTIONS)

Because you are in a function, you don't need to use a unique variable name.

RedBeard0531 · 2023-04-05T18:36:06Z

CMakeLists.txt

-        string(REPLACE "-Wno-" "-W" check_flag ${flag})
+function(append_source_file_compile_options file property)
+    get_source_file_property(SRC_PROPS_${file} ${file} COMPILE_OPTIONS)
+    if ("${SRC_PROPS_${file}}" STREQUAL "NOTFOUND" OR "${SRC_PROPS_${file}}" STREQUAL "")


Suggested change

if ("${SRC_PROPS_${file}}" STREQUAL "NOTFOUND" OR "${SRC_PROPS_${file}}" STREQUAL "")

if (NOT OLD_OPTS)

The if() command in cmake is wierd and magical but somehow works out well with the rest of cmake.

…piler-warnings

michael-wb · 2023-04-05T21:09:20Z

Created #6474 to address the data race failure in LangBindHelper_HandoverBetweenThreads in core tests for the ubuntu2004-encryption-tsan task. The other failure is the known asymmetric sync issue.

Disabled unused but set variable warnings in CMakeLists.txt

d9fa991

cla-bot bot added the cla: yes label Apr 4, 2023

github-actions bot assigned michael-wb Apr 4, 2023

Updated changelog

71bb665

michael-wb requested review from fealebenpae and danieltabacaru April 4, 2023 13:00

fealebenpae approved these changes Apr 4, 2023

View reviewed changes

Michael Wilkerson-Barker added 3 commits April 4, 2023 10:06

Fix warnings in existing files

74c74f4

Remove global no-unused-but-set warnings and add back for parser files

6460bc9

Disable the unused-but-set warning on the 3rd party Catch library

8bb2ca7

michael-wb requested review from fealebenpae and tgoyne April 4, 2023 17:46

tgoyne reviewed Apr 4, 2023

View reviewed changes

RedBeard0531 requested changes Apr 5, 2023

View reviewed changes

Updates from review and added append_source_file_compile_options func…

e7292f5

…tion

michael-wb requested review from tgoyne and RedBeard0531 April 5, 2023 16:31

tgoyne approved these changes Apr 5, 2023

View reviewed changes

kiburtse suggested changes Apr 5, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

src/realm/parser/CMakeLists.txt Outdated Show resolved Hide resolved

RedBeard0531 approved these changes Apr 5, 2023

View reviewed changes

Michael Wilkerson-Barker added 2 commits April 5, 2023 16:05

Updates from review - beefed up append compile options function

ed0edf9

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-com…

565d584

…piler-warnings

kiburtse approved these changes Apr 5, 2023

View reviewed changes

Some simplification of append compile options function

9563e27

Added status back to append compile options for debug

ff727ad

Michael Wilkerson-Barker added 2 commits April 5, 2023 21:24

check compile flag variable is global

fab8375

Cleanup after debugging

c404a42

michael-wb merged commit 15543d5 into master Apr 6, 2023

michael-wb deleted the mwb/fix-compiler-warnings branch April 6, 2023 03:37

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler warnings with MacOS Clang 14.0.3 #6467

Fix compiler warnings with MacOS Clang 14.0.3 #6467

michael-wb commented Apr 4, 2023 •

edited

Loading

kiburtse commented Apr 4, 2023

michael-wb commented Apr 4, 2023

kiburtse commented Apr 4, 2023

tgoyne Apr 4, 2023

michael-wb Apr 4, 2023 •

edited

Loading

michael-wb Apr 4, 2023

tgoyne Apr 4, 2023

michael-wb Apr 5, 2023

RedBeard0531 Apr 5, 2023

tgoyne Apr 5, 2023

RedBeard0531 left a comment

RedBeard0531 Apr 5, 2023

michael-wb commented Apr 5, 2023

RedBeard0531 Apr 5, 2023

RedBeard0531 Apr 5, 2023

michael-wb Apr 5, 2023

michael-wb commented Apr 5, 2023 •

edited

Loading

	function(add_cxx_flag_if_supported flag)
	if(flag MATCHES "^-Wno-")
	# Compilers ignore unknown -Wno-foo flags, so look for -Wfoo instead.
	string(REPLACE "-Wno-" "-W" check_flag ${flag})
	else()
	set(check_flag ${flag})
	endif()

	check_cxx_compiler_flag(${check_flag} HAVE${check_flag})
	if(HAVE${check_flag})
	add_compile_options($<$<COMPILE_LANGUAGE:CXX>:${flag}>)
	endif()
	endfunction()

	get_source_file_property(SRC_PROPS_${file} ${file} COMPILE_OPTIONS)
	get_source_file_property(OLD_OPTS ${file} COMPILE_OPTIONS)

	if ("${SRC_PROPS_${file}}" STREQUAL "NOTFOUND" OR "${SRC_PROPS_${file}}" STREQUAL "")
	if (NOT OLD_OPTS)

Fix compiler warnings with MacOS Clang 14.0.3 #6467

Fix compiler warnings with MacOS Clang 14.0.3 #6467

Conversation

michael-wb commented Apr 4, 2023 • edited Loading

What, How & Why?

☑️ ToDos

kiburtse commented Apr 4, 2023

michael-wb commented Apr 4, 2023

kiburtse commented Apr 4, 2023

Choose a reason for hiding this comment

michael-wb Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb commented Apr 5, 2023 • edited Loading

michael-wb commented Apr 4, 2023 •

edited

Loading

michael-wb Apr 4, 2023 •

edited

Loading

michael-wb commented Apr 5, 2023 •

edited

Loading