build: yet another C++20 migration #2341

scarf005 · 2023-02-13T05:16:20Z

Summary

SUMMARY: Build "Migrate to C++20"

Purpose of change

continuation of #1624 and #2003

Describe the solution

all the fixes from [WIP] Upgrade cpp version #1624, and more
ported Build Android in C++17 mode CleverRaven/Cataclysm-DDA#64305 by @BrettDong
rewrote quantity<V, U> using c++20 features (concept, <=>)

Todos

Didn't astyle have issues with the spaceship operator?
We can't just change our entire code style, it would prevent most C++ ports from DDA.
JSON errors interfere with checks, needs a rebase
Fix android build failure
Migrate lit.cfg to C++20
Isn't macOS 11 still maintained?
Docs will need an update regarding minimal compiler versions, minimal macOS version. Docs still mention macOS 10, g++ 4.8 and clang 3.8

scarf005 · 2023-03-27T09:11:11Z

i hate c++

olanti-p · 2023-03-28T17:53:22Z

Didn't astyle have issues with the spaceship operator? We can't just change our entire code style, it would prevent most C++ ports from DDA.
JSON errors interfere with checks, needs a rebase
Unclear what's up with Android build
Does the Windows/cygwin build work? It's not in CI
Do the MXE Windows releases work? They're not in PR checks
Has tidy been migrated? lit.cfg still has the c++17 switch
Isn't macOS 11 still maintained?
Docs will need an update regarding minimal compiler versions, minimal macOS version. Docs still mention macOS 10, g++ 4.8 and clang 3.8

scarf005 · 2023-03-28T22:31:57Z

Didn't astyle have issues with the spaceship operator?

yes. I had to add // *NOPAD* to ignore these lines. they're fixed in 3.2 but contributors will have to manually download them atm, because latest version on ubuntu is 3.1

Unclear what's up with Android build

I'm suspecting it's related to gradle, investigating

Isn't macOS 11 still maintained?

could you clarify? should i downgrade the requirement to macOS 11?

Does Windows/cygwin build work? It's not in CI

Do the MXE Windows releases work? They're not in PR checks

should i add build matrix target or ask for help to other windows users? i'm unable to access windows atm

We can't just change our entire code style, it would prevent most C++ ports from DDA.

do you mean the auto returns?

olanti-p · 2023-03-28T23:35:26Z

could you clarify? should i downgrade the requirement to macOS 11?

I was concerned about dropping support for osx 11, which people still may be using, but our download stats show up as single digits for OSX, and there's no way to tell which exact versions people use, so I guess it doesn't matter much...

should i add build matrix target or ask for help to other windows users? i'm unable to access windows atm

Cygwin can be fixed at a later time if needed then, it's rather obscure and usually works as long as the MSYS build works.

MXE build doesn't need Windows to compile: it's a cross-build from Linux, and can be tested on Github by enabling actions in settings of a BN fork and then pushing to upload. It should at least compile, it's our main windows release builds at the moment (the "non-msvc" ones)

Cataclysm-BN/.github/workflows/release.yml

Lines 87 to 100 in 8761440

    
           - name: Windows Tiles x64 
        
             mxe: x86_64 
        
             mxe_apt: x86-64 
        
             artifact: windows-tiles-x64 
        
             os: ubuntu-20.04 
        
             ext: zip 
        
             content: application/zip 
        
           - name: Windows Tiles x32 
        
             mxe: i686 
        
             mxe_apt: i686 
        
             artifact: windows-tiles-x32 
        
             os: ubuntu-20.04 
        
             ext: zip 
        
             content: application/zip

do you mean the auto returns?

I meant as in moving to other formatting tools that support the spaceship operator, like clang-format, in a way that would end up reformatting tons of lines.

scarf005 · 2023-03-28T23:40:48Z

i see. i think we could wait until astyle 3.2 (which supports spaceship operator) , and until then ignore those lines since we won't be using spaceship operator more than 10 times.

as for the android build failure, this looks like due to outdated gradle version.

src/units_def.h

Coolthulhu · 2023-04-24T08:04:19Z

src/weather_gen.cpp

-                                    NUM_SEASONS ) ) % NUM_SEASONS;
-    const size_t next_season = ( current_season + 1 ) % NUM_SEASONS;
+                                    num_seasons ) ) % num_seasons;
+    const size_t next_season = ( current_season + 1 ) % num_seasons;


Is it safe to % unsigned?

i guess it's okay, as divider(num_seasons) is 4. i could change them to int.

scarf005 · 2023-04-24T15:31:42Z

(copied from discord)
so clang-tidy is dying because it fails to parse <=> operator.
looks like it's fixed in clang 13? ...which means we'll have to also update our custom clang-tidy program.
that looks like a lot of work...
https://bugs.llvm.org/show_bug.cgi?id=47511

possible solution:

rollback and manually define all these operators again (yuck)
try compiling cata-clang-tidy plugin without using homebrew clang12 (will take ages)
ignore this happened (constant clang-tidy death)

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 <[email protected]>

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 <[email protected]>

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

Co-authored-by: Brett Dong <[email protected]>

fixed by using legacy comparison operators clang-tidy 12 used by cata-clang-tidy cannot handle spaceship operator see: cataclysmbnteam#2341 (comment)

scarf005 · 2023-04-25T14:46:43Z

used old operators again, confirmed it compiles in clang++

scarf005 · 2023-04-25T22:33:58Z

oh it's no use trying to remove <=>, c++ stdlib uses them anyway

 + '[' -f build/tools/clang-tidy-plugin/libCataAnalyzerPlugin.so ']'
+ LD_PRELOAD=build/tools/clang-tidy-plugin/libCataAnalyzerPlugin.so
+ clang-tidy -quiet tests/vehicle_test.cpp
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: clang-tidy -quiet tests/algo_test.cpp
1.	<eof> parser at end of file
2.	While analyzing stack: 
	#0 Calling std::lexicographical_compare_three_way at line /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_vector.h:2056:14
	#1 Calling std::operator<=> at line /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/predefined_ops.h:45:16
	#2 Calling __gnu_cxx::__ops::_Iter_less_iter::operator() at line /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_algo.h:1809:8
	#3 Calling std::__insertion_sort at line /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_algo.h:1854:2
	#4 Calling std::__final_insertion_sort at line /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_algo.h:1940:4
	#5 Calling std::__sort at line /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_algo.h:4820:7
	#6 Calling std::sort at line 22
	#7 Calling check_cycle_finding at line 35
	#8 Calling C_A_T_C_H_T_E_S_T_1

olanti-p

If this is still stuck on the tidy plugin, we could try porting DDA's migration to LLVM 16
CleverRaven/Cataclysm-DDA#65381
At the very least, this would simplify our tidy workflow since clang-tidy-16 supports plugins "out of the box" (we don't have to compile a hacked version for local testing).

scarf005 · 2024-08-29T00:02:11Z

closing as this is stuck on #3028 which also is unlikely to be merged in near future

github-actions bot added src changes related to source code. tests changes related to tests labels Feb 13, 2023

scarf005 closed this Mar 27, 2023

scarf005 reopened this Mar 27, 2023

scarf005 force-pushed the c++20 branch from 98feb4a to de1bb6f Compare March 27, 2023 00:33

scarf005 marked this pull request as ready for review March 27, 2023 00:34

scarf005 changed the title ~~Build: yet another C++20 migration~~ [CR] Build: yet another C++20 migration Mar 27, 2023

scarf005 marked this pull request as draft March 27, 2023 00:40

scarf005 force-pushed the c++20 branch 2 times, most recently from 0c0ee58 to 3956633 Compare March 28, 2023 15:50

scarf005 marked this pull request as ready for review March 28, 2023 15:50

scarf005 requested review from Coolthulhu, joveeater and olanti-p March 28, 2023 16:11

scarf005 force-pushed the c++20 branch from 4bca5c8 to f2382fd Compare March 28, 2023 22:56

scarf005 mentioned this pull request Mar 28, 2023

locally test C++20 scarf005/Cataclysm-BN#13

Merged

scarf005 force-pushed the c++20 branch 5 times, most recently from c0c78b1 to 91233b5 Compare March 29, 2023 06:31

cataclysmbnteam deleted a comment from github-actions bot Mar 29, 2023

Coolthulhu reviewed Apr 24, 2023

View reviewed changes

src/units_def.h Outdated Show resolved Hide resolved

Coolthulhu reviewed Apr 24, 2023

View reviewed changes

scarf005 force-pushed the c++20 branch 2 times, most recently from 90f4c29 to f24a9a0 Compare April 24, 2023 11:25

scarf005 and others added 12 commits April 25, 2023 23:28

build: 17 -> 20

976545f

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

135dd5b

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

ci: bump lit version to c++20

bce9029

refactor: use default equality

969b01c

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

079e05d

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

fe3dc28

build: ignore modernize-use-nullptr

3f4ca1e

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

build: android in c++20

835aacf

Co-authored-by: Brett Dong <[email protected]>

refactor: apply some idea suggestions

b3011ec

ci: bump java langauge version to 11

dca584e

scarf005 force-pushed the c++20 branch from f24a9a0 to f2cf776 Compare April 25, 2023 14:44

scarf005 force-pushed the c++20 branch from f2cf776 to dca584e Compare April 25, 2023 22:34

scarf005 dismissed joveeater’s stale review via dca584e May 7, 2023 20:36

scarf005 mentioned this pull request May 19, 2023

host compiled custom-clang-tidy #1968

Closed

olanti-p reviewed Jul 21, 2023

View reviewed changes

scarf005 marked this pull request as draft August 5, 2023 04:24

scarf005 closed this Aug 29, 2024

This was referenced Oct 3, 2024

test: migrate clang-tidy to LLVM 18 #5497

Merged

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: yet another C++20 migration #2341

build: yet another C++20 migration #2341

scarf005 commented Feb 13, 2023 •

edited

Loading

scarf005 commented Mar 27, 2023

olanti-p commented Mar 28, 2023 •

edited

Loading

scarf005 commented Mar 28, 2023 •

edited

Loading

olanti-p commented Mar 28, 2023 •

edited

Loading

scarf005 commented Mar 28, 2023

Coolthulhu Apr 24, 2023

scarf005 Apr 24, 2023

scarf005 commented Apr 24, 2023 •

edited

Loading

scarf005 commented Apr 25, 2023

scarf005 commented Apr 25, 2023

olanti-p left a comment

scarf005 commented Aug 29, 2024

build: yet another C++20 migration #2341

build: yet another C++20 migration #2341

Conversation

scarf005 commented Feb 13, 2023 • edited Loading

Summary

Purpose of change

Describe the solution

Todos

scarf005 commented Mar 27, 2023

olanti-p commented Mar 28, 2023 • edited Loading

scarf005 commented Mar 28, 2023 • edited Loading

olanti-p commented Mar 28, 2023 • edited Loading

scarf005 commented Mar 28, 2023

Coolthulhu Apr 24, 2023

Choose a reason for hiding this comment

scarf005 Apr 24, 2023

Choose a reason for hiding this comment

scarf005 commented Apr 24, 2023 • edited Loading

scarf005 commented Apr 25, 2023

scarf005 commented Apr 25, 2023

olanti-p left a comment

Choose a reason for hiding this comment

scarf005 commented Aug 29, 2024

scarf005 commented Feb 13, 2023 •

edited

Loading

olanti-p commented Mar 28, 2023 •

edited

Loading

scarf005 commented Mar 28, 2023 •

edited

Loading

olanti-p commented Mar 28, 2023 •

edited

Loading

scarf005 commented Apr 24, 2023 •

edited

Loading