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

Disable buggy warnings #65542

Merged
merged 3 commits into from
May 30, 2023
Merged

Conversation

Arkaeriit
Copy link
Contributor

@Arkaeriit Arkaeriit commented May 8, 2023

Summary

Infrastructure "Fix GCC warnings"

Purpose of change

When compiling the game, a lot of warnings (especial dangling reference warnings) were triggered. This is probably because I am using GCC 13.

Describe the solution

I performed some slight modifications to the code that don't change the final behavior but that please the compiler.

Describe alternatives you've considered

Testing

I tested in game the result of those changes to ensure that the game behavior is unchanged.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Crafting / Construction / Recipes Includes: Uncrafting / Disassembling EOC: Effects On Condition Anything concerning Effects On Condition Info / User Interface Game - player communication, menus, etc. Map / Mapgen Overmap, Mapgen, Map extras, Map display NPC / Factions NPCs, AI, Speech, Factions, Ownership Player Faction Base / Camp All about the player faction base/camp/site Vehicles Vehicles, parts, mechanics & interactions labels May 8, 2023
src/activity_item_handling.cpp Outdated Show resolved Hide resolved
src/activity_item_handling.cpp Outdated Show resolved Hide resolved
src/crafting.cpp Outdated Show resolved Hide resolved
src/crafting.cpp Outdated Show resolved Hide resolved
src/crafting.cpp Outdated Show resolved Hide resolved
src/map.cpp Outdated Show resolved Hide resolved
src/map.cpp Outdated Show resolved Hide resolved
src/map.cpp Outdated Show resolved Hide resolved
src/map.cpp Outdated Show resolved Hide resolved
src/map.cpp Outdated Show resolved Hide resolved
@irwiss
Copy link
Contributor

irwiss commented May 8, 2023

How does this affect performance?

At a glance this will switch a ton of valid references (at least for the lifetime of their call site) to making copies, for example stuff like map::get_known_connections will now potentially copy ter_t(vector + 2 sets + minor stuff) in an inner loop and itself is called in a loop, the change in output.cpp seems just plain weird

@Arkaeriit
Copy link
Contributor Author

Hi irwiss.

I am quite a beginner at C++ so I might be doing some bad mistakes. If I understand the issue, GCC is not happy with using references from the chain of function calls as something needed for the rest of the function.

The performance cost of my solution is indeed quite bad. Thank you for pointing it out, I will try to find a better way.

What is the issue with the change in output.cpp? As I am not very accustomed with C++, there might be something I am missing.

@irwiss
Copy link
Contributor

irwiss commented May 8, 2023

string_id and int_id act like pointers in this codebase(react to operator* and arrow), they'll call the .obj() to return a reference to the pointed-at object, these objects are valid after json data is loaded (happens just before savegame data loads), until game quits to main menu, i don't know of any exceptions to this

I'm not sure what gcc13 complains about as i don't use it but likely the solution is to squelch that particular warning, either in makefile or somehow annotate .obj() function

the output.cpp change is weird because you change from (allegedly) dangling reference warning to (allegedly) a dangling pointer, if that pleases the compiler then the compiler seems to be wrong

src/faction_camp.cpp Outdated Show resolved Hide resolved
@Arkaeriit Arkaeriit force-pushed the dangling-reference-warning branch from 944f0e5 to 8943784 Compare May 8, 2023 16:20
@github-actions github-actions bot added the Code: Build Issues regarding different builds and build environments label May 8, 2023
@Arkaeriit
Copy link
Contributor Author

I disabled that warning in the makefile. Is that what you wanted?

@irwiss
Copy link
Contributor

irwiss commented May 8, 2023

That's one way although that completely eliminates even the may be valid results

May be wrapping the specific function that raises the false positives in push/ignored/pop pragmas is an option, cursory google search appears to return a bunch of projects that resorted to pragma wraps for view/reference wrapper kind of classes

i'm not sure why it's specifically ter/furn it complains about though, as it should complain about all types going through generic_factory::obj() 🤷‍♂️

@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels May 8, 2023
@andrei8l
Copy link
Contributor

That's one way although that completely eliminates even the may be valid results

-Wdangling-reference is very broken as evidenced by the numerous upstream bug reports and ASan will catch any valid issue anyway. Disabling the warning is the right solution for now.

Fixes #65449

I would also add -Wno-c++20-compat even though you've fixed the only trigger.

Arkaeriit added 2 commits May 27, 2023 18:57
As this is a keyword in C++20, it should be used
to name a variable.
GCC 13 reports false positives of this warning.
@Arkaeriit Arkaeriit force-pushed the dangling-reference-warning branch from a45aa9b to b9fad0a Compare May 27, 2023 16:58
As suggested by andrei8l
@Arkaeriit Arkaeriit force-pushed the dangling-reference-warning branch from b9fad0a to db128ad Compare May 27, 2023 17:00
@Arkaeriit Arkaeriit changed the title Fix various warnings Disable buggy warnings May 27, 2023
@dseguin dseguin merged commit 9ffcd2b into CleverRaven:master May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments Crafting / Construction / Recipes Includes: Uncrafting / Disassembling EOC: Effects On Condition Anything concerning Effects On Condition Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display NPC / Factions NPCs, AI, Speech, Factions, Ownership Player Faction Base / Camp All about the player faction base/camp/site Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants