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

Resolves several compile warnings when comparing different int signedness #1755

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

alsed
Copy link

@alsed alsed commented Dec 20, 2024

advances #713 and eliminates several compilation warnings concerning comparison between integers of different signedness.

this resolve a compilation warning about comparison of expressions wih different signedness.
…r_Client_Check function

this avoid a compilation warning about comparison of integer expressions of different signedness.
this avoid compilation warnings about comparison expressions of different signedness.
- resolves a compilation warning about comparison expressions
of different signedness.
- setupstyle() uses getcurrentstylename() to retrieve name.
- correction in the message log.
- resolved a comp warning about comparison of different signedness
- new function created: calculate_packed_xz(),
used by const CPosition& CLevelGraph::vertex_position() and bool CLevelGraph::inside()
- aditional minor resolves
…dness

- resolves a warning compilation in ConvertTextureFormat(),
modified to a for loop range, this avoid integers comparisions.

- resolves warnings when int comparing with unsigned macro R__NUM_SUN_CASCADES,
in render_phase_sun.cpp.

- resolves warnings when int comparing with unsigned macro R__NUM_PARALLEL_CONTEXTS,
in r2_loader.cpp

- var "b_count" converted to unsigned to resolve warnings within for loops,
in AnimationKeyCalculate.h

- resolves warnings when for loops compare signed vs unsigned, in r__sector.cpp
@github-actions github-actions bot added Renderer AI Artificial Intelligence Sound UI labels Dec 20, 2024
…ness (2)

- advances OpenXRay#713 and eliminates all warnings concerning this operation:
- in R_dsgraph_structure::load() / r__dsgraph_build.cpp
- var 'vcnt' converted to u32 in R_Backend_DBG.cpp
- several for loops in light.cpp
- several for loops in in DetailManager_CACHE.cpp
- var 'filled_blocks' converted from int to i32, in SoundRender_Emitter.cpp
- in CSoundRender_Core::create() / SoundRender_Core.cpp,
explicit convertion of var 'sg_SourceType' to int
- for loop in CollectorPacked::CollectorPacked() / xrCDB_Collector.cpp
- several for loops in Frustum.cpp
@alsed alsed changed the title Resolves several compile warnings when comparing different int signedness Resolves all compile warnings when comparing different int signedness Dec 21, 2024
@alsed alsed changed the title Resolves all compile warnings when comparing different int signedness Resolves several compile warnings when comparing different int signedness Dec 21, 2024
Copy link
Member

@Xottab-DUTY Xottab-DUTY left a comment

Choose a reason for hiding this comment

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

LGTM!
I didn't checked xrAICore and xrSound yet.
The main thing to correct here is replace unsigned int with u32.

src/Layers/xrRender/AnimationKeyCalculate.h Outdated Show resolved Hide resolved
src/Layers/xrRender/R_Backend_DBG.cpp Outdated Show resolved Hide resolved
src/Layers/xrRender/light.cpp Outdated Show resolved Hide resolved
@@ -866,7 +866,7 @@ void xrServer::Server_Client_Check(IClient* CL)
return;
};

if (CL->process_id == GetCurrentProcessId())
if (CL->process_id == static_cast<u32>(GetCurrentProcessId()))
Copy link
Member

Choose a reason for hiding this comment

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

On Windows process_id variable has DWORD type and GetCurrentProcessId() returns DWORD too.
On Linux this variable has type pid_t and GetCurrentProcessId() is a macro to getpid function that returns pid_t.

Why does it throw a warning here??

Copy link
Author

@alsed alsed Dec 30, 2024

Choose a reason for hiding this comment

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

As with all the changes in this commit, the warning is for comparing a signed int with an unsigned.

Edit:
Is for pid_t :

warning: comparison of integer expressions of different signedness: ‘u32’ {aka ‘unsigned int’} and ‘__pid_t’ {aka ‘int’} [-Wsign-compare]
  869 |     if (CL->process_id == GetCurrentProcessId())
      |         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~``

Copy link
Member

@Xottab-DUTY Xottab-DUTY Dec 31, 2024

Choose a reason for hiding this comment

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

Try this. Will it fix the warning?

Suggested change
if (CL->process_id == static_cast<u32>(GetCurrentProcessId()))
if (CL->process_id == static_cast<xrpid_t>(GetCurrentProcessId()))

Copy link
Author

@alsed alsed Dec 31, 2024

Choose a reason for hiding this comment

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

The warning persist, xrpid_t is consider an int.
How about converting process_id to int for this comparison? unsigned to signed should be safer
if (static_cast<int>(CL->process_id) == GetCurrentProcessId())

@Xottab-DUTY Xottab-DUTY added Code Quality good first issue Good start for beginners that want to contribute. labels Dec 30, 2024
@alsed
Copy link
Author

alsed commented Dec 30, 2024

Changes requested already made.
I'm thinking I should change all unsigned int to u32 since that's what they really are comparing to... let me know if I proceed.

The last 2 warning regarding this (not included in the commit yet) are these:

/xray-16/src/xrCore/LocatorAPI.cpp:362:30: warning: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  362 |             VERIFY(read_byte == dwSize);
/xray-16/src/xrCore/string_concatenations.cpp:111:33: warning: comparison of integer expressions of different signedness: ‘u32’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare]
  111 |     VERIFY(overrun_string_index != -1);

I'm not sure the best way to resolve them, but we can do it and eliminate all.


VERIFY(iFloor((source_position.z - box_z) / cell_size + .5f) < (int)m_row_length);
Copy link
Member

Choose a reason for hiding this comment

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

This VERIFY was removed. Needs to be returned.

Copy link
Author

Choose a reason for hiding this comment

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

Ops! ...returned.

@@ -115,7 +115,7 @@ CSound* CSoundRender_Core::create(pcstr fName, esound_type sound_type, int game_
auto* snd = xr_new<CSound>(handle);

snd->g_type = game_type;
if (game_type == sg_SourceType)
if (game_type == static_cast<int>(sg_SourceType))
Copy link
Member

Choose a reason for hiding this comment

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

game_type is u32, so no need to cast sg_SourceType to int.

Suggested change
if (game_type == static_cast<int>(sg_SourceType))
if (game_type == sg_SourceType)

@@ -85,7 +85,7 @@ class CSoundRender_Emitter final : public CSound_emitter
std::atomic<Task*> prefill_task{};

size_t current_block{};
int filled_blocks{};
u32 filled_blocks{};
Copy link
Member

Choose a reason for hiding this comment

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

This was actually made as int on purpose in case of underflows.
It's better to leave it as is for now and examine the code that operates on this variable to make sure there are no underflows.

Suggested change
u32 filled_blocks{};
int filled_blocks{};

Copy link
Author

Choose a reason for hiding this comment

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

Alright, comment added because I have the memory of a goldfish

Copy link
Member

Choose a reason for hiding this comment

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

No comment here, please.
Other person is working on xrSound currently and it might cause conflicts.

Copy link
Author

Choose a reason for hiding this comment

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

Ok

- all `unsigned int` changed to `u32`
- on SoundRender_Emitter.h - var `filled_blocks` reverted to `int` as requested.
- reverted changes to `level_graph_inline.h`
- proposal for `Server_Client_Check()` on `xrServer.cpp`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Artificial Intelligence Code Quality good first issue Good start for beginners that want to contribute. Renderer Sound UI
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants