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

Fix: don't allow deleting SettingDesc to prevent compilers getting confused #9390

Closed
wants to merge 2 commits into from

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 20, 2021

Fixes #9386.
Alternative to #9389. Closes #9389.

Motivation / Problem

MacOS said: BOOOO
GCC said: fuck this shit

Similar to #9389, this shows that the core problem is not so much the std::string, but more the default dtor.

Description

Please don't accept this PR. I just wanted it here to list it for history, and hopefully someone can discover a proper solution. This triggers a bunch of warnings, and is just a bit nasty.

And no, you cannot do ~SettingDesc() = delete, as then sub-classes cannot use the ctor of SettingDesc to create its object.

The first commit removes unique_ptr from the code-base, and might in general be a good idea to accept in the code-base. I can create a separate PR for that if this is wanted.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
TrueBrain added 2 commits Jun 20, 2021
While at it, remove the "unique_ptr", as these static lists are
never destructed anyway (well, on close of course, but that the
OS will clean up for us nicely).
…nfused

SettingDesc is only used in static lists, so never destructed
anyway (well, on close of course, but that the OS will clean up
for us nicely). And having a dtor, which the compiler sees is
never used btw, causes compilers to start optimizing that, and ..
get lost somewhere.

GCC complaints with:
  note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without

clang on MacOS just crashes (with bus error 10, read: out of memory).

This commit produces tons of warnings, as it is not really meant
to be accepted upstream. More to show that the problem of OpenTTD#9386
is not just "std::string" -> "std::string_view", but much deeper
in fact.
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Jun 21, 2021

Rb has a fix pending that is vastly better than this approach. Closing this!

Loading

@TrueBrain TrueBrain closed this Jun 21, 2021
@TrueBrain TrueBrain deleted the settings-use-span branch Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

1 participant