Signed/Unsigned Mismatch Inconsistency

Hi!

I don’t know if it would be better to post this in the forums or here. I would like to get the attention of some of the programmers at Epic though. If I should move this to the forum, please let me know.

I’d like to discuss signed/unsigned comparison warnings with the C++ compiler. Looking at the toolchain .cs files, looks like there’s been some internal debate. I would really like to get this out on the table, get this settled by Epic folks, and get some consistency in the compiler settings, particularly in cross-platform environments.

First, I’ll start with “the problem” – that is, why I’m bothering you about this in the first place. Our development team works primarily in Linux, but we support both Linux and Windows with our UE4-based software. On Linux, Clang doesn’t seem to care if we do checks between signed and unsigned types. On Windows, VC++ does care… but only sometimes. This adds overhead when Linux devs submit their feature branch pull requests, as we have to find/fix all of the sign issues.

I figured a simple “fix” to make things more consistent would be to enable sign checking on Clang. But when I turn this on, I get 100 build errors building the first 17 files, and the compile aborts. Unexpectedly, not all of the errors were in Linux-specific code.

I started digging into it, and this is what I found to be the current situation:

  • On Windows, WindowsPlatformCompilerSetup.h disables warning C4389. This apparently prevents scenarios like Stats2.h, line 1313, which assigns a bool based on a comparison between a uint64 and an int32, from generating a warning.

  • However, nothing Windows-side disables warning C4018, which generates warnings in a situation like for(uint32 i=0; i < Arr.Num(); ++i).

  • Note that for the life of me, I can’t seem to fathom the difference between the two above warnings. In two cases of comparing a signed and unsigned int (one in a for loop, one in a bool assignment), different warnings are generated.

  • VCToolChain.cs passes “-Wno-sign-compare” to Clang when compiling with Clang on Windows (that is, when Target.WindowsPlatform.Compiler == WindowsCompiler.Clang; I don’t know for sure if this is making a Windows build with Clang, or if this is for cross-platform compiling), turning off sign warnings.

  • On Linux, apparently Clang doesn’t generate sign warning by default. Nothing currently enables it, resulting in a notable difference between Linux and Windows error checking.

  • LinuxToolChain.cs uses “-Wno-sign-compare” with GCC (not the default path) to disable sign warnings.

  • On Mac, MacToolChain.cs at one time added “-Wsign-compare” to its compiler, enabling sign warnings. The comment reads “// fed up of not seeing the signed/unsigned warnings we get on Windows - lets enable them here too.” This is a relatable sentiment. However, the line is presently (as of 4.21) commented out, presumably defaulting back to no sign checks.

It seems to me that most of UE4 is geared toward suppressing the signed/unsigned comparison warnings.

Personally, I would argue the warnings should be enabled, and the issues (carefully) fixed. Comparison between signed and unsigned types is easy to do accidentally (as our team has proven), and can cause subtle and hard to find/fix bugs. If the developers at Epic would agree to this point, I would love for an issue to be created, the warnings to be enabled on on all platforms and compilers, and the resulting issues fixed (or suppressed in special cases where needed). In fact, I would be happy to consider doing this work and submitting it as a pull request.

If, however, Epic feels that the warnings are generally unnecessary and bothersome (as seems to be the case heretofore), I would like to see both of the Windows warnings disabled for the sake of consistency, particularly between platforms, and let the chips fall where they may. :slight_smile:

If anyone has input on the matter, I would love to hear opinions or arguments in either direction.

Thanks for your time and attention!

So? Did you submit pull request? Because i don’t see problem with that and Epic staff will look on it and possibly discuss.

You also got 2nd option… submit bug raport ;p since you could consider this a bug + bug raport usally return email explaining why something is not a bug, but if it will pass as a bug it will be added to UE4 official bug tracker.

I did not! I expect it to be a fair amount of work, and I don’t want to waste my time doing it if Epic are of the mindset that the warnings should be disabled instead of fixed.

I actually started to submit a bug report, and I may yet do that. But when it started asking for repro steps and things of that nature, I realized I might rather have this be a discussion than a single request with a single, limited response. This format or the forums would let others (like yourself) contribute their thoughts and opinions as well, which may be helpful in coming to a decision.

Don’t be shy, posting change it self is not that hard specially if they are simple, make a fork in github, commit change (you can edit code in github text editor too) and submit pull request with description, it can be fixed along later while discussing it or Epic could actually add there own solution.

As for reproduction steps, make reproduction steps to show warning difference, you can even include step for switch OS, it can be very general with simple example code

You more likely to reach Epic this way and actully find anwser or even impact. I would probably also say you more likely to reach epic of Feedback section of forums then here as this place is more about shering knowledge.

But again don’t be shy, if you have ability to do pull request then go right ahead instead of wasting time here and be actully now to make step forward first and make things happen. i see lot of people here complaining that something been not changed, sometimes very simple, yet they ignore fact that they have ability that epic gives to suggest direct code changes in GitHub. You can look up how process looks like yourself:

https://github.com/EpicGames/UnrealEngine/pulls

I’m sorry, I didn’t communicate clearly. I expect resolving the, I’m guessing, multiple hundreds of sign mismatches to be a fair amount of work. You’re correct in that submitting a pull request is straightforward. I’ve done it several times.

And, yes, I considered submitting a bug report, and may go that route still. I fear you may be right in that that would be the more likely way to get a response from Epic. I will try the forums route as well.

In the olden days of UE2 and early UE3 development, the unprog email list was where developers communicated with each other and Epic. This seemed like the closest analog to those days, but it does seem like Epic doesn’t often respond directly to AnswerHub posts.

Thank you for your suggestions! I appreciate it!