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

clang++-10 compiler warnings / formatting issues #192

Closed
capnm opened this issue Sep 2, 2020 · 12 comments
Closed

clang++-10 compiler warnings / formatting issues #192

capnm opened this issue Sep 2, 2020 · 12 comments

Comments

@capnm
Copy link

capnm commented Sep 2, 2020

May show some unexpected code side effects. Godot 3.2 tip + Cory's FastNoise + modules/voxel/

scons use_llvm=yes use_lld=yes ... 2> >(tee ../log/xxx-$date.log >&2)

modules/voxel/voxel_buffer.cpp:124:19: warning: comparison of integers of different signs: 'int' and 'const uint32_t' (aka 'const unsigned int') [-Wsign-compare]
        ERR_FAIL_COND(sx > MAX_SIZE || sy > MAX_SIZE || sz > MAX_SIZE);
                      ~~ ^ ~~~~~~~~
./core/error_macros.h:278:16: note: expanded from macro 'ERR_FAIL_COND'
                if (unlikely(m_cond)) {                                                                            \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
modules/voxel/voxel_buffer.cpp:124:36: warning: comparison of integers of different signs: 'int' and 'const uint32_t' (aka 'const unsigned int') [-Wsign-compare]
        ERR_FAIL_COND(sx > MAX_SIZE || sy > MAX_SIZE || sz > MAX_SIZE);
                                       ~~ ^ ~~~~~~~~
./core/error_macros.h:278:16: note: expanded from macro 'ERR_FAIL_COND'
                if (unlikely(m_cond)) {                                                                            \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
modules/voxel/voxel_buffer.cpp:124:53: warning: comparison of integers of different signs: 'int' and 'const uint32_t' (aka 'const unsigned int') [-Wsign-compare]
        ERR_FAIL_COND(sx > MAX_SIZE || sy > MAX_SIZE || sz > MAX_SIZE);
                                                        ~~ ^ ~~~~~~~~
./core/error_macros.h:278:16: note: expanded from macro 'ERR_FAIL_COND'
                if (unlikely(m_cond)) {                                                                            \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
3 warnings generated.
modules/voxel/streams/voxel_block_serializer.cpp:27:42: warning: comparison of integers of different signs: 'const int' and 'const uint32_t' (aka 'const unsigned int') [-Wsign-compare]
                ERR_FAIL_COND_V_MSG(pos.x < 0 || pos.x >= VoxelBuffer::MAX_SIZE, 0, "Invalid voxel metadata X position");
                                                 ~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~
./core/error_macros.h:335:16: note: expanded from macro 'ERR_FAIL_COND_V_MSG'
                if (unlikely(m_cond)) {                                                                                                                        \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
modules/voxel/streams/voxel_block_serializer.cpp:28:42: warning: comparison of integers of different signs: 'const int' and 'const uint32_t' (aka 'const unsigned int') [-Wsign-compare]
                ERR_FAIL_COND_V_MSG(pos.y < 0 || pos.y >= VoxelBuffer::MAX_SIZE, 0, "Invalid voxel metadata Y position");
                                                 ~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~
./core/error_macros.h:335:16: note: expanded from macro 'ERR_FAIL_COND_V_MSG'
                if (unlikely(m_cond)) {                                                                                                                        \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
modules/voxel/streams/voxel_block_serializer.cpp:29:42: warning: comparison of integers of different signs: 'const int' and 'const uint32_t' (aka 'const unsigned int') [-Wsign-compare]
                ERR_FAIL_COND_V_MSG(pos.z < 0 || pos.z >= VoxelBuffer::MAX_SIZE, 0, "Invalid voxel metadata Z position");
                                                 ~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~
./core/error_macros.h:335:16: note: expanded from macro 'ERR_FAIL_COND_V_MSG'
                if (unlikely(m_cond)) {                                                                                                                        \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
modules/voxel/streams/voxel_block_serializer.cpp:77:32: warning: comparison of integers of different signs: 'long' and 'const size_t' (aka 'const unsigned long') [-Wsign-compare]
                CRASH_COND_MSG((dst - p_dst) > metadata_size, "Wrote block metadata out of expected bounds");
                                ~~~~~~~~~~~  ^ ~~~~~~~~~~~~~
./core/error_macros.h:313:16: note: expanded from macro 'CRASH_COND_MSG'
                if (unlikely(m_cond)) {                                                                                                     \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
modules/voxel/streams/voxel_block_serializer.cpp:94:32: warning: comparison of integers of different signs: 'long' and 'const size_t' (aka 'const unsigned long') [-Wsign-compare]
                CRASH_COND_MSG((dst - p_dst) > metadata_size, "Wrote voxel metadata out of expected bounds");
                                ~~~~~~~~~~~  ^ ~~~~~~~~~~~~~
./core/error_macros.h:313:16: note: expanded from macro 'CRASH_COND_MSG'
                if (unlikely(m_cond)) {                                                                                                     \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
modules/voxel/streams/voxel_block_serializer.cpp:99:31: warning: comparison of integers of different signs: 'long' and 'const size_t' (aka 'const unsigned long') [-Wsign-compare]
        CRASH_COND_MSG((dst - p_dst) != metadata_size,
                        ~~~~~~~~~~~  ^  ~~~~~~~~~~~~~
./core/error_macros.h:313:16: note: expanded from macro 'CRASH_COND_MSG'
                if (unlikely(m_cond)) {                                                                                                     \
                             ^~~~~~
./core/typedefs.h:319:41: note: expanded from macro 'unlikely'
#define unlikely(x) __builtin_expect(!!(x), 0)
                                        ^
6 warnings generated.
modules/voxel/streams/voxel_stream_region_files.cpp:408:14: warning: unused function 'to_varray' [-Wunused-function]
static Array to_varray(const Vector3i &v) {
             ^
modules/voxel/streams/voxel_stream_region_files.cpp:431:13: warning: unused function 'from_json_varray' [-Wunused-function]
static bool from_json_varray(Array a, Vector3i &v) {
            ^
2 warnings generated.
@capnm
Copy link
Author

capnm commented Sep 2, 2020

Some of the formatting issues:

 File formatting checks (file_format.sh)
53s
  shell: /usr/bin/bash -e {0}
Run bash ./misc/scripts/file_format.sh
  bash ./misc/scripts/file_format.sh
  shell: /usr/bin/bash -e {0}

*** The following differences were found between the code and the formatting rules:

diff --git a/modules/voxel/doc/classes/README.md b/modules/voxel/doc/classes/README.md
index 0cf337c..aef06b8 100644
--- a/modules/voxel/doc/classes/README.md
+++ b/modules/voxel/doc/classes/README.md
@@ -1,2 +1,2 @@
 Doc classes will appear here when generating, using Godot's doctool.
-See https://docs.godotengine.org/en/stable/development/cpp/custom_modules_in_cpp.html#writing-custom-documentation
\ No newline at end of file
+See https://docs.godotengine.org/en/stable/development/cpp/custom_modules_in_cpp.html#writing-custom-documentation
diff --git a/modules/voxel/editor/graph/voxel_graph_node_inspector_wrapper.cpp b/modules/voxel/editor/graph/voxel_graph_node_inspector_wrapper.cpp
index fed9220..41a77f9 100644
--- a/modules/voxel/editor/graph/voxel_graph_node_inspector_wrapper.cpp
+++ b/modules/voxel/editor/graph/voxel_graph_node_inspector_wrapper.cpp
@@ -94,4 +94,4 @@ bool VoxelGraphNodeInspectorWrapper::_get(const StringName &p_name, Variant &r_r
 	}
 
 	return true;
-}
\ No newline at end of file
+}
diff --git a/modules/voxel/util/direct_static_body.cpp b/modules/voxel/util/direct_static_body.cpp
index c389100..7850010 100644
--- a/modules/voxel/util/direct_static_body.cpp
+++ b/modules/voxel/util/direct_static_body.cpp
@@ -112,4 +112,4 @@ void DirectStaticBody::set_debug(bool enabled, World *world) {
 
 		_debug_mesh_instance.destroy();
 	}
-}
\ No newline at end of file
+}

*** Aborting, please fix your commit(s) with 'git commit --amend' or 'git rebase -i <hash>'
##[error]Process completed with exit code 1.

@capnm capnm changed the title clang++-10 compiler warnings clang++-10 compiler warnings / formatting issues Sep 2, 2020
@capnm
Copy link
Author

capnm commented Sep 2, 2020

Another one

2020-09-02T17:06:43.4993560Z clang++ -o modules/voxel/streams/voxel_stream.osx.opt.tools.64.o -c -std=gnu++14 -Wno-inconsistent-missing-override -g1 -O2 -arch x86_64 -mmacosx-version-min=10.9 -isysroot /Applications/Xcode_11.6.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -Wall -Werror -DDEBUG_ENABLED -DTYPED_METHOD_BIND -DOSX_ENABLED -DUNIX_ENABLED -DGLES_ENABLED -DAPPLE_STYLE_KEYS -DCOREAUDIO_ENABLED -DCOREMIDI_ENABLED -DGL_SILENCE_DEPRECATION -DPTRCALL_ENABLED -DTOOLS_ENABLED -DGDSCRIPT_ENABLED -DMINIZIP_ENABLED -DZSTD_STATIC_LINKING_ONLY -DGLAD_ENABLED -DGLES_OVER_GL -DMODULE_ASSIMP_ENABLED -DMODULE_BMP_ENABLED -DMODULE_BULLET_ENABLED -DMODULE_CAMERA_ENABLED -DMODULE_CSG_ENABLED -DMODULE_CVTT_ENABLED -DMODULE_DDS_ENABLED -DMODULE_ENET_ENABLED -DMODULE_ETC_ENABLED -DMODULE_FREETYPE_ENABLED -DMODULE_GDNATIVE_ENABLED -DMODULE_GDSCRIPT_ENABLED -DMODULE_GRIDMAP_ENABLED -DMODULE_HDR_ENABLED -DMODULE_JPG_ENABLED -DMODULE_JSONRPC_ENABLED -DMODULE_MBEDTLS_ENABLED -DMODULE_MOBILE_VR_ENABLED -DMODULE_NOISE_ENABLED -DMODULE_OGG_ENABLED -DMODULE_OPUS_ENABLED -DMODULE_PVR_ENABLED -DMODULE_RECAST_ENABLED -DMODULE_REGEX_ENABLED -DMODULE_SQUISH_ENABLED -DMODULE_STB_VORBIS_ENABLED -DMODULE_SVG_ENABLED -DMODULE_TGA_ENABLED -DMODULE_THEORA_ENABLED -DMODULE_TINYEXR_ENABLED -DMODULE_UPNP_ENABLED -DMODULE_VHACD_ENABLED -DMODULE_VISUAL_SCRIPT_ENABLED -DMODULE_VORBIS_ENABLED -DMODULE_VOXEL_ENABLED -Ithirdparty/libpng -Ithirdparty/glad -Ithirdparty/zstd -Ithirdparty/zlib -Iplatform/osx -I. modules/voxel/streams/voxel_stream.cpp
2020-09-02T17:06:43.9395100Z modules/voxel/streams/voxel_block_serializer.cpp:101:21: error: conversion from 'const size_t' (aka 'const unsigned long') to 'const Variant' is ambiguous
2020-09-02T17:06:43.9598450Z                                         .format(varray(metadata_size, (int)(dst - p_dst))));
2020-09-02T17:06:43.9700660Z                                                        ^~~~~~~~~~~~~
2020-09-02T17:06:43.9808550Z ./core/error_macros.h:314:115: note: expanded from macro 'CRASH_COND_MSG'
2020-09-02T17:06:43.9916070Z                         _err_print_error(FUNCTION_STR, __FILE__, __LINE__, "FATAL: Condition \"" _STR(m_cond) "\" is true.", DEBUG_STR(m_msg)); \
2020-09-02T17:06:44.0002290Z                                                                                                                                        ^~~~~
2020-09-02T17:06:44.0009000Z ./core/error_macros.h:116:26: note: expanded from macro 'DEBUG_STR'
2020-09-02T17:06:44.0110320Z #define DEBUG_STR(m_msg) m_msg
2020-09-02T17:06:44.0214730Z                          ^~~~~
2020-09-02T17:06:44.0316160Z ./core/variant.h:251:2: note: candidate constructor
2020-09-02T17:06:44.0417600Z         Variant(bool p_bool);
2020-09-02T17:06:44.0519030Z         ^
2020-09-02T17:06:44.0621150Z ./core/variant.h:252:2: note: candidate constructor
2020-09-02T17:06:44.0723730Z         Variant(signed int p_int); // real one
2020-09-02T17:06:44.0724310Z         ^
2020-09-02T17:06:44.0763130Z ./core/variant.h:253:2: note: candidate constructor
2020-09-02T17:06:44.0848030Z         Variant(unsigned int p_int);
2020-09-02T17:06:44.0848340Z         ^
2020-09-02T17:06:44.0862470Z ./core/variant.h:259:2: note: candidate constructor
2020-09-02T17:06:44.0863270Z         Variant(signed short p_short); // real one
2020-09-02T17:06:44.0864210Z         ^
2020-09-02T17:06:44.0904440Z ./core/variant.h:260:2: note: candidate constructor
2020-09-02T17:06:44.0905200Z         Variant(unsigned short p_short);
2020-09-02T17:06:44.0905770Z         ^
2020-09-02T17:06:44.0925170Z ./core/variant.h:261:2: note: candidate constructor
2020-09-02T17:06:44.0925890Z         Variant(signed char p_char); // real one
2020-09-02T17:06:44.0926480Z         ^
2020-09-02T17:06:44.0927010Z ./core/variant.h:262:2: note: candidate constructor
2020-09-02T17:06:44.0932170Z         Variant(unsigned char p_char);
2020-09-02T17:06:44.0950930Z         ^
2020-09-02T17:06:44.0951860Z ./core/variant.h:263:2: note: candidate constructor
2020-09-02T17:06:44.0952220Z         Variant(int64_t p_int); // real one
2020-09-02T17:06:44.0954300Z         ^
2020-09-02T17:06:44.0955150Z ./core/variant.h:264:2: note: candidate constructor
2020-09-02T17:06:44.0955950Z         Variant(uint64_t p_int);
2020-09-02T17:06:44.0973170Z         ^
2020-09-02T17:06:44.1028830Z ./core/variant.h:265:2: note: candidate constructor
2020-09-02T17:06:44.1034260Z         Variant(float p_float);
2020-09-02T17:06:44.1035070Z         ^
2020-09-02T17:06:44.1040390Z ./core/variant.h:266:2: note: candidate constructor
2020-09-02T17:06:44.1042380Z         Variant(double p_double);
2020-09-02T17:06:44.1043120Z         ^
2020-09-02T17:06:44.1044380Z 1 error generated.
2020-09-02T17:06:44.1145550Z scons: *** [modules/voxel/streams/voxel_block_serializer.osx.opt.tools.64.o] Error 1
2020-09-02T17:06:45.2371160Z scons: building terminated because of errors.

@capnm
Copy link
Author

capnm commented Sep 2, 2020

Python formatting:

2020-09-02T18:54:13.4278342Z ##[group]Run bash ./misc/scripts/black_format.sh
2020-09-02T18:54:13.4278558Z �[36;1mbash ./misc/scripts/black_format.sh�[0m
2020-09-02T18:54:13.4317841Z shell: /usr/bin/bash -e {0}
2020-09-02T18:54:13.4317996Z ##[endgroup]
2020-09-02T18:54:13.4390444Z Formatting Python files...
2020-09-02T18:54:15.7354068Z reformatted /home/runner/work/godot/godot/modules/voxel/config.py
2020-09-02T18:54:15.7354445Z reformatted /home/runner/work/godot/godot/modules/voxel/SCsub
2020-09-02T18:54:15.7355097Z reformatted /home/runner/work/godot/godot/modules/voxel/doc/tools/build.py
2020-09-02T18:54:15.7355342Z reformatted /home/runner/work/godot/godot/modules/voxel/doc/tools/xmlproc.py
2020-09-02T18:54:16.0718796Z All done! ✨ 🍰 ✨
2020-09-02T18:54:16.0719013Z 5 files reformatted, 190 files left unchanged.
2020-09-02T18:54:16.1036584Z 
2020-09-02T18:54:16.1037486Z *** The following differences were found between the code and the formatting rules:
2020-09-02T18:54:16.1037797Z 
2020-09-02T18:54:16.1057562Z diff --git a/modules/voxel/SCsub b/modules/voxel/SCsub
2020-09-02T18:54:16.1057724Z index 14ee9d6..e8449b0 100644
2020-09-02T18:54:16.1057995Z --- a/modules/voxel/SCsub
2020-09-02T18:54:16.1058159Z +++ b/modules/voxel/SCsub
2020-09-02T18:54:16.1058426Z @@ -1,44 +1,44 @@
2020-09-02T18:54:16.1058641Z -Import('env')
2020-09-02T18:54:16.1058917Z -Import('env_modules')
2020-09-02T18:54:16.1059067Z +Import("env")
2020-09-02T18:54:16.1059208Z +Import("env_modules")

...

2020-09-02T18:54:16.1254473Z *** Aborting, please fix your commit(s) with 'git commit --amend' or 'git rebase -i <hash>'
2020-09-02T18:54:16.1257240Z ##[error]Process completed with exit code 1.

@capnm
Copy link
Author

capnm commented Sep 2, 2020

modules/voxel/generators/graph/voxel_generator_graph.cpp:506:8: error: variable 'v' set but not used [-Werror=unused-but-set-variable]
  506 |  float v;
      |        ^

@Zylann
Copy link
Owner

Zylann commented Sep 7, 2020

Unfortunately it's going to be hard for me to fix these. I can only compile with MSVC and MinGW locally, and none of them give me any of these warnings...

@capnm
Copy link
Author

capnm commented Sep 7, 2020

I just mentioned that in case you someday want to integrate the module into Godot :-) The above breaks the current Godot CI build.

We're making a curated version of Godot 3.x because I think that unfortunately nobody really cares about the bugs anymore and the hyped 4.x will be useless for low-end hardware in schools or by children for years ...

I kept my branch with your module that passed the ci here. You can have a look if you want.
https://github.com/capnm/godot/tree/capnm-32x-voxel
CI errors e.g. https://github.com/capnm/godot/runs/1063241748
https://github.com/capnm/godot/actions/runs/236563050

@Zylann
Copy link
Owner

Zylann commented Sep 7, 2020

In fact I'm setting up CI currently, since I discovered how to use it to also make builds for multiple platforms earlier on a GDNative plugin. Going to fix the warnings there because they are treated as errors.

So far I see it takes long to build (30 min for one platform without scons cache) but that's enough for me. Only problem so far is that the tests spin endlessly after hitting GLES3 failure (which works on the official Godot repo for reasons beyond my understanding).

Also, this: godotengine/godot#36690

in case you someday want to integrate the module into Godot

That would be cool, but I don't see this happening anytime soon.

We're making a curated version of Godot 3.x

You're maintaining a fork of 3.x with extra modules in it?

@capnm
Copy link
Author

capnm commented Sep 8, 2020

In fact I'm setting up CI currently, since I discovered how to use it to also make builds for multiple platforms earlier on a GDNative plugin. Going to fix the warnings there because they are treated as errors.

Thanks, please keep going :-) Treating warnings as errors is generally a good idea. Some languages
(Golang) for various philosophical reasons do not support warnings at all;)

So far I see it takes long to build (30 min for one platform without scons cache) but that's enough for me. Only problem so far is that the tests spin endlessly after hitting GLES3 failure (which works on the official Godot repo for reasons beyond my understanding).

I had no such problems, I cloned the Godot repo, activated the CI for my 3.2 rebased test-branch and deactivated some compiler warnings, since fixing of the old Godot code in so many places would make it unmaintainable against the official 3.2 tip. It uses the scons cache and usually runs faster than I can drink my coffee, honestly I don't care...

Also, this: godotengine/godot#36690

The new compilers are getting smarter to spot the issues. I'm not a C++ expert at all, but casting the value to something the same size or larger (which the Variant class supports) seems to fix the issue.

You're maintaining a fork of 3.x with extra modules in it?

That's the plan. I currently only need a usable version of Godot for my CS courses that I am planning for next year. Version 3.x has better network support (multiplayer!) and you now, the kids love Minecraft crafting voxels ;-)

@Zylann
Copy link
Owner

Zylann commented Sep 8, 2020

Just merged CI for Windows and Linux, which includes fixes for warnings.
However that's now in the godot3.2.3 branch. I merged lots of changes to master and it has warnings again on Linux. Gonna have to fix them later.

@Zylann
Copy link
Owner

Zylann commented Sep 9, 2020

Warnings should be fixed now in master too.

@Zylann
Copy link
Owner

Zylann commented Sep 11, 2020

And they are back. Gonna fix them soon.
In fact, expect GCC warnings to come in master from time to time, cuz I develop on Windows and neither MSVC nor MinGW give me these warnings. I'll fix them eventually tho, cuz now I have CI to check them.
So I don't know what to do about this issue?

@Zylann
Copy link
Owner

Zylann commented Aug 15, 2022

This should be fixed now. There is a CI now so errors like this will get fixed soon after pushing to the repo, as it is impractical to test all compilers and platforms locally beforehand

@Zylann Zylann closed this as completed Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants