build: C++20 migration (again^3) #5514

scarf005 · 2024-10-05T15:00:53Z

Checklist

Required

I wrote the PR title in conventional commit format.
I ran the code formatter.
I linked any relevant issues using github keyword syntax so it can be closed automatically.
I have committed my changes to new branch that isn't main so it won't cause conflict when updating main branch later.

Optional

This is a PR that modifies build process or code organization.
- Please document the changes in the appropriate location in the doc/ folder.
- If documentation for this feature or process does not exist, please write it.
- If the change alters versions of software required to build or work with the game, please document it.

Purpose of change

continuation of #1624 and #2003 and #2341

applied <=> operator and concepts to showcase new C++20 features
use sweet sweet ranges

Describe the solution

reapplied commits from build: yet another C++20 migration #2341
disabled some filesystem tests that were constantly failing on windows (refer to 10d6c53 (#5514))

Describe alternatives you've considered

completely overhaul codebase to use std::filesystem? MSVC tests results are really weird, fails even on ascii filesystem tests and not sure why

Testing

intentionally ignoring clang-tidy failures because

the automated changes are too large
currently preparing for following PR that applies all available automated fixes

Additional context

also 1) fix incomplete achievement_requirement 2) remove user defined copy constructor: error: definition of implicit copy assignment operator for 'point_with_value' is deprecated because it has a user-declared copy constructor [-Werror,-Wdeprecated-copy] Co-authored-by: AngelicosPhosphoros <angelicos.phosphoros@protonmail.com>

cannot use spaceship operator until llvm/llvm-project#43670

1. add `concepts_utility.h` for storing useful concepts related headers. 2. uses `Arithmatic` concepts to ensure they're numbers. 3. uses spaceship operator for comparison. 4. uses auto and arrow returns to make templates readable. 5. make fabs and fmod constexpr. Co-authored-by: Coolthulhu <Coolthulhu@gmail.com>

it works as a false positive on spaceship operators `<=>`

OrenAudeles

Looks fine, just have some questions. The effect the Arithmetic concept has on the templates in units_def.h is crazy!

OrenAudeles · 2024-10-06T03:44:09Z

src/mod_manager_ui.cpp

        info += colorize( vgettext( "Maintainer", "Maintainers", mod->maintainers.size() ),
-                          c_light_blue ) + u8":\u00a0"/*non-breaking space*/ + enumerate_as_string( mod->maintainers ) + "\n";
+                          c_light_blue ) +
+                // HACK: Cannot fix that without switching whole project to std::u8string.


How painful would it be to change everything to u8strings?
Might not need to update everything to u8strings, but user-facing stuff might need to be. It's a shame that it "just worked"(tm) in pre-20 but is specialized to u8strings now.

looks like there's no easy solution:

https://stackoverflow.com/questions/56833000/c20-with-u8-char8-t-and-stdstring/65468019#65468019

https://old.reddit.com/r/cpp/comments/1csf6n4/c20_why_stdu8string_lacks_support_in_other_c/

https://stackoverflow.com/questions/55556200/convert-between-stdu8string-and-stdstring

https://stackoverflow.com/questions/13444930/is-the-u8-string-literal-necessary-in-c11

OrenAudeles · 2024-10-06T03:53:47Z

src/vehicle_use.cpp

@@ -137,7 +137,7 @@ void vehicle::add_toggle_to_opts( std::vector<uilist_entry> &options,
                              name );
    options.emplace_back( -1, allow, key, msg );

-    actions.emplace_back( [ = ] {
+    actions.emplace_back( [ =, this ] {


Does adding this to the captures change anything about how the lambda functions?

since c++20 implicit this caputure has been made error

okay coolie 👍

OrenAudeles · 2024-10-06T07:24:31Z

the Clang-tidy (tiles) / build (pull_request) failure is emitting this error. Not sure what to do about that one since it's in sol files.

and the Cataclysm Windows build (Cmake + MSVC) / Build (pull_request) failure is in tests/filesystem_test.cpp at the filesystem_utf8[_comp|_decomp] tests. filesystem_ascii passes just fine, so something is still weird about filesystem tests. The failure point is all on the same line checking for directory existence right after calling bool remove_tree( const std::string &path ) so it's likely failing there.

passing string directly borks filesystem test on windows

like what's the problem

rationale: - spent past 6 hours trying to fix this and failed - currently not worth the effort considering it's not used anywhere in-game

chaosvolt · 2024-10-07T18:11:05Z

Oren asked me in the discord server to uncomment the lines that were causing problems and test if it works on mine or not, it indeed does fail as expected on windows for me:

OrenAudeles

Other than clang-tidy whining about a static assertion error that doesn't come up while actually building, and claims of not being able to find <lua.h>, both involving sol I don't see anything else preventing merging. The clang-tidy errors are nonsense.

Will need to look at the filesystem_test issues revolving around remove_tree and dir_exists since they only show up as an issue on the windows build. Thankfully remove_tree is only used as part of this one test and not the game proper.

OrenAudeles · 2024-10-07T23:00:36Z

src/achievement.cpp

@@ -9,7 +9,6 @@
 #include "character.h"
 #include "color.h"
 #include "debug.h"
-#include "enums.h"
 #include "event.h"
 #include "event_statistics.h"


#include "event_statistics.h" and #include "json.h" are no longer needed in this file since they've been added to achievement.h

OrenAudeles · 2024-10-07T23:41:02Z

src/iuse.cpp

 struct object_names_collection;

+struct extended_photo_def : public JsonDeserializer, public JsonSerializer {


Moving the definition of extended_photo_def up to here isn't really relevant to the PR scope.

iirc it was somewhat related to build error but i forgot

OrenAudeles · 2024-10-07T23:44:43Z

src/mod_manager_ui.cpp

        info += colorize( vgettext( "Maintainer", "Maintainers", mod->maintainers.size() ),
-                          c_light_blue ) + u8":\u00a0"/*non-breaking space*/ + enumerate_as_string( mod->maintainers ) + "\n";
+                          c_light_blue ) +


extraneous + at the end of this line

Suggested change

c_light_blue ) +

c_light_blue )

OrenAudeles · 2024-10-07T23:55:43Z

src/sol/forward.hpp

@@ -34,7 +34,7 @@

 // beginning of sol/version.hpp

-#include <sol/config.hpp>
+#include "./config.hpp"


Other than this line that clang-tidy was complaining about the only changes to this file are whitespace. Might be able to discard all changes to this file so clang-tidy skips it and doesn't get mad about something stupid.

Could probably keep it as <sol/config.hpp> in that case since it has no effect on compilation. Not entirely sure, though :/

OrenAudeles · 2024-10-08T00:04:35Z

src/sol/sol.hpp

@@ -38,7 +38,7 @@ static_assert(false, "sol.hpp must be included via catalua_sol.h");

 // beginning of sol/version.hpp

-#include <sol/config.hpp>
+#include "./config.hpp"


Similarly to forward.hpp the changes in this file are mostly cosmetic whitespace removals that don't change functionality. Could change back to #include <sol/config.hpp> while getting rid of all changes to this file and clang-tidy will probably shut up about it.

OrenAudeles · 2024-10-08T00:06:36Z

android/app/src/main/java/org/libsdl/app/SDL.java

-                throw se;
-            }
-        }        
+            System.loadLibrary(libraryName);


Is removing the try-catch block here safe to do?

yes, because the original try-catch re-throws exceptions which means it essentially does nothing

chaosvolt

Compiled and loaded a save.
Saved game and deleted world without issue.
Created a game without issue.
Started new game in new world, chargen and entering game world also works fine.
Saved this new game for good measure too.

github-actions bot added docs PRs releated to docs page src changes related to source code. tests changes related to tests labels Oct 5, 2024

scarf005 force-pushed the build/c++20-again branch from 0d09b35 to 5be4ecc Compare October 5, 2024 15:36

scarf005 marked this pull request as ready for review October 5, 2024 16:15

scarf005 force-pushed the build/c++20-again branch from a58f4e0 to 02ac5dc Compare October 6, 2024 01:30

scarf005 and others added 13 commits October 6, 2024 12:24

build: 17 -> 20

1dbae86

build: disable zero-as-null-pointer-constant

cc35f0b

cannot use spaceship operator until llvm/llvm-project#43670

ci: bump lit version to c++20

f710870

refactor: use default equality

b9df3b4

ci: explicitly use C++20 for clang-tidy

3543adb

build: add c++20 flag to Android.mk

db9782f

build: ignore modernize-use-nullptr

4c56db3

it works as a false positive on spaceship operators `<=>`

refactor: apply some idea suggestions

770d942

ci: bump java langauge version to 11

bfb806d

test: run all clang-tidy check even if it fails

cb3be58

test: problemetic checks

f4166d0

scarf005 force-pushed the build/c++20-again branch from 02ac5dc to f4b8b82 Compare October 6, 2024 03:27

fix: attempt to fix MSVC build error

52f6230

scarf005 force-pushed the build/c++20-again branch from f4b8b82 to 52f6230 Compare October 6, 2024 03:28

OrenAudeles reviewed Oct 6, 2024

View reviewed changes

scarf005 added 2 commits October 6, 2024 17:45

fix: rollback to old filesystem code

d5797df

passing string directly borks filesystem test on windows

fix: use relative import to prevent clang-tidy complaints

5729df4

scarf005 force-pushed the build/c++20-again branch 3 times, most recently from d10a685 to d2c81eb Compare October 6, 2024 12:57

test: add more captures

296467d

like what's the problem

scarf005 force-pushed the build/c++20-again branch from d2c81eb to 296467d Compare October 6, 2024 13:13

test: skip remove_tree and remove_directory test atm

10d6c53

rationale: - spent past 6 hours trying to fix this and failed - currently not worth the effort considering it's not used anywhere in-game

scarf005 force-pushed the build/c++20-again branch from d716a5a to 10d6c53 Compare October 6, 2024 15:09

OrenAudeles approved these changes Oct 8, 2024

View reviewed changes

chaosvolt approved these changes Oct 8, 2024

View reviewed changes

scarf005 merged commit fd85e05 into cataclysmbnteam:main Oct 8, 2024
11 of 12 checks passed

scarf005 deleted the build/c++20-again branch October 8, 2024 06:25

This was referenced Oct 8, 2024

refactor: use C++20 string::{starts,ends}_with #5535

Merged

refactor: use concepts more #5537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: C++20 migration (again^3) #5514

build: C++20 migration (again^3) #5514

scarf005 commented Oct 5, 2024 •

edited

Loading

OrenAudeles left a comment

OrenAudeles Oct 6, 2024

scarf005 Oct 8, 2024

OrenAudeles Oct 6, 2024

scarf005 Oct 6, 2024

OrenAudeles Oct 6, 2024

OrenAudeles commented Oct 6, 2024

chaosvolt commented Oct 7, 2024

OrenAudeles left a comment

OrenAudeles Oct 7, 2024

OrenAudeles Oct 7, 2024

scarf005 Oct 8, 2024

OrenAudeles Oct 7, 2024

OrenAudeles Oct 7, 2024

OrenAudeles Oct 8, 2024

OrenAudeles Oct 8, 2024

scarf005 Oct 8, 2024

chaosvolt left a comment

		struct object_names_collection;

		struct extended_photo_def : public JsonDeserializer, public JsonSerializer {

build: C++20 migration (again^3) #5514

build: C++20 migration (again^3) #5514

Conversation

scarf005 commented Oct 5, 2024 • edited Loading

Checklist

Required

Optional

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

OrenAudeles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrenAudeles commented Oct 6, 2024

chaosvolt commented Oct 7, 2024

OrenAudeles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaosvolt left a comment

Choose a reason for hiding this comment

scarf005 commented Oct 5, 2024 •

edited

Loading