aliens

Author Topic: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019  (Read 3269 times)

Offline Skybuck

  • Colonel
  • ****
  • Posts: 223
    • View Profile
https://github.com/MeridianOXC/OpenXcom/blob/oxce-plus/src/Battlescape/Map.cpp

1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\Map.cpp(606,71): warning C4804: '-': unsafe use of type 'bool' in operation

   bool unitFromBelow = false;
   bool unitFromAbove = false;

_camera->convertMapToScreen(unitTile->getPosition() + Position(0,0, (-unitFromBelow) + (+unitFromAbove)), &tileScreenPosition);

I am not sure what the intent of this code is but it's suspect ! =D

negating a booleang is suspect... perhaps use !unitFromBelow ? perhaps code thinks these are integers ?

Offline Yankes

  • Commander
  • *****
  • Posts: 3194
    • View Profile
Re: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019
« Reply #1 on: May 23, 2022, 11:18:05 am »
bool is integral type that can have only 0 and 1, using `-` is fine in this context whole expression is normal number.

Offline Skybuck

  • Colonel
  • ****
  • Posts: 223
    • View Profile
Re: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019
« Reply #2 on: May 24, 2022, 01:51:19 pm »
LOL, Compiler disagrees with you.

Delphi can also have strange boolean values, like -1 or 255, boolean are a bit weird.


Offline Skybuck

  • Colonel
  • ****
  • Posts: 223
    • View Profile
Re: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019
« Reply #4 on: May 24, 2022, 06:55:13 pm »
The code is hard to read and should really be written as:

_camera->convertMapToScreen
(
   unitTile->getPosition() +
   Position
   (
      0, 0, (-unitFromBelow) + (+unitFromAbove)
   ),
   &tileScreenPosition
);

Now it becomes more clear what it is doing. It's trying to initialize the Z value.

Apperently it's trying to convert the booleans to an integer value. This means boolean can be 0 or 1. -bool can therefore be -1. Converting -1 can lead to 255 if converted to char or 65535 if converted to word or large positive for other unsigned integers. So it depends on what C++ converts a boolean to when minus is applied. Since booleans are always positive I can see a risk how a compiler write might always convert it to an unsigned integer, in that case -bool can indeed become unsafe.

I am not a C/C++ language expert so I don't know what the C/C++ expert says about applieing minus to a boolean and what type it should be converted too. From the link which you provided it seems it will be converted to whatever the destination is. So if position would be changed to unsigned integers this code would no longer work as intended.

Also PBRT requires c/c++ 2017, and your link is toward c/c++ 2011. Perhaps rules for minus boolean changed in c/c++ 2017, but for now it only seems to be a warning from visual studio compiler to help programmers avoid nasty to find programming mistakes.

Finally there are some changes for prefix and postfix operators to booleans:

https://docs.microsoft.com/en-us/cpp/cpp/bool-cpp?view=msvc-170

These are no longer allowed. Most likely a sign of things to come.

Now for a solution how to get rid of this warning. Probably typecast the boolean first to an integer, then apply the minus sign to be on the safe side.

Offline Yankes

  • Commander
  • *****
  • Posts: 3194
    • View Profile
Re: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019
« Reply #5 on: May 24, 2022, 07:19:12 pm »
Did you even read links I post? en.cppreference.com is up to date with C++23.
This is fully defined behavior in C++, all arithmetic in C++ is done as `int` if arguments have smaller size than `int`. `bool` is part of this system.

Beside read what you link, page show `--` and `++` is now banned from usage, not `+` and `-`.


Offline Skybuck

  • Colonel
  • ****
  • Posts: 223
    • View Profile
Re: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019
« Reply #6 on: May 25, 2022, 12:27:54 am »
I read it, it's messy as hell, you need to be a robot to parse that, I see "since 2011" for relevant parts about bool.

I have no idea update status of document, is it a mix of c++ languages ?

You fail to see my point, -- and ++ banned, + and - might follow next.

I am not going to parse robot weird c++ language document, except relevant parts to bool and integer, don't care about floats, etc.

I do not know what since 2011 means.

Anyway how would these insanely complex document signal to end of life of some support, for example -- and ++ is it in there or is it left as a surprise to the programmer ?

Is there some terminology, like since 2011 till 2017 or since 2011 till infinity or now... multiple ways to interpret all of this.

Like since 2011, but read other documents for limitations lol.
« Last Edit: May 25, 2022, 12:31:15 am by Skybuck »

Offline Yankes

  • Commander
  • *****
  • Posts: 3194
    • View Profile
Re: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019
« Reply #7 on: May 25, 2022, 01:07:01 pm »
Then how do you plan arguing what is correct and what is not in C++ when do you can't even read SIMPLIFIED C++ documentation?

This " (since C++11)" is referring ONLY to one word "unscoped", noting else.

"mix of c++ languages", no, this is state for C++20 but simply note where given change was introduce to language or remove from it.


And you fail to see point why `--` and  `++` is removed, reason is simple as result is pointless as `++b` result will always be `true` and in `--b` will invert state.
In case of `-` and `+` there is no problems like this, as standard clearly state this is converted to `int` with values `0` and `1` and then apply this operators.
If you try do `b = -b;` then yes this have no sense and compiler should warning about this, but in case of my code this is no the case as I effective do `int i = -b;` that have clear and understandable meaning.

If you do not see difference between this two cases I do not see point discussion this any further.


btw No other compiler show errors in this case (GCC, Clang and ICC), and IMHO VS is incorrect in that case and it should be fixed as this is false positive warning.
https://godbolt.org/z/ad3PG48Mq
Even if I add aggressive warnings in other compilers they do not show no warning

Offline Skybuck

  • Colonel
  • ****
  • Posts: 223
    • View Profile
Re: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019
« Reply #8 on: May 26, 2022, 01:48:00 am »
Then how do you plan arguing what is correct and what is not in C++ when do you can't even read SIMPLIFIED C++ documentation?

Look at how other languages deal with it and what your gut instinct tells you.

Quote
This " (since C++11)" is referring ONLY to one word "unscoped", noting else.

mix of c++ languages", no, this is state for C++20 but simply note where given change was introduce to language or remove from it.

... ?

Quote
And you fail to see point why `--` and  `++` is removed, reason is simple as result is pointless as `++b` result will always be `true` and in `--b` will invert state.

You a hypocrit. -- and ++ is not ok/banned/removed/can't even test it. While you fine with - and + which does mathetically more or less the same. In Delphi it is not allowed without a typecast. Plus results can be different depending on boolean or bytebool. Where bytebool is negative for false. So this code would then not work for bytebool.

Basically the code depends on how a language translates false into a numerical value, this can vary from language to language. When writing code it's important to keep in mind how other languages might deal with this.

Quote
In case of `-` and `+` there is no problems like this, as standard clearly state this is converted to `int` with values `0` and `1` and then apply this operators.


Which is weird to auto convert a boolean to some other arbitrary type. Why not convert it to a string and use - and + as string operators ?

Quote
If you try do `b = -b;` then yes this have no sense and compiler should warning about this, but in case of my code this is no the case as I effective do `int i = -b;` that have clear and understandable meaning.

This is not clear from code, the conversion from boolean to integer is implicit and highly confusing and completely depends on what the language does, languages do different things in this case.

Quote
If you do not see difference between this two cases I do not see point discussion this any further.

There are even more cases as you indicated, and you yourself state that it doesn't make sense. My Delphi programming experience has shown me that converting booleans to anything else is very tricky and can cause problems and you yourself indicate there are problems in c/c++ where it does weird things like b = -b;

Also consider how easy it is to overlook it and think of it as negation which apperently it's not really: -boolean(0) stay at 0.

Quote
btw No other compiler show errors in this case (GCC, Clang and ICC), and IMHO VS is incorrect in that case and it should be fixed as this is false positive warning.
https://godbolt.org/z/ad3PG48Mq
Even if I add aggressive warnings in other compilers they do not show no warning

Perhaps Microsoft knows something we don't, that it might be removed in the future or there is some weird bug potential.

Anyway it does warn for another potential pitfall which I don't see how it can be triggered, the + between both booleans can overflow, the booleans are converted to 8 bit values.

So here the language seems inconsistent, is it converted to int32 or int8 ? strange stuff.

For now I agree with you the code is "relatively" safe in C/C++ since it follows this weird standard, but it ain't pretty and it's an annoying warning messages, getting rid of it would be nice.

You all spent so much time getting rid of warning messages, didn't see a single one, this is the only one ! =D
« Last Edit: May 26, 2022, 01:49:44 am by Skybuck »

Offline Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 8597
    • View Profile
Re: OXCE Plus: Map.cpp has unsafe use of bool according to vs2019
« Reply #9 on: May 26, 2022, 09:49:15 am »
The code is written as intended.

It is 100% compatible with the c++ specification and works correctly on all 4 compilers we're currently using.

End of the story.