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

Use C++ style casts #80

Merged
merged 2 commits into from
Nov 4, 2024
Merged

Conversation

CrackedMatter
Copy link
Contributor

No description provided.

@cursey cursey requested a review from angelfor3v3r November 3, 2024 19:57
Copy link
Collaborator

@angelfor3v3r angelfor3v3r left a comment

Choose a reason for hiding this comment

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

The casts inside align_up and align_down won't work this way, could you change them so the checks pass? I believe special care must be taken when T is a pointer vs some integer type.

Copy link
Collaborator

@angelfor3v3r angelfor3v3r left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

@angelfor3v3r angelfor3v3r merged commit 4b062f6 into cursey:main Nov 4, 2024
5 checks passed
@A1mDev
Copy link

A1mDev commented Dec 18, 2024

https://github.com/alliedmodders/sourcemod/actions/runs/12385271125/job/34571228199 - It seems these changes are not being compiled on clang

@angelfor3v3r
Copy link
Collaborator

https://github.com/alliedmodders/sourcemod/actions/runs/12385271125/job/34571228199 - It seems these changes are not being compiled on clang

Could it be because of clang 8? It seems like some concepts were introduced with clang 10 & clang 16.

@A1mDev
Copy link

A1mDev commented Dec 18, 2024

It seems because of stdc++17 =(

@angelfor3v3r
Copy link
Collaborator

It seems because of stdc++17 =(

Makes sense. We develop for C++23 but there is some polyfills for C++20 (not sure how well it works, I never tried it):

parser.add_argument('--polyfill', action='store_true',

@A1mDev
Copy link

A1mDev commented Dec 18, 2024

Everything compiles fine except for this code - https://github.com/cursey/safetyhook/blob/main/include/safetyhook/utility.hpp#L17-L26

@angelfor3v3r
Copy link
Collaborator

angelfor3v3r commented Dec 18, 2024

Everything compiles fine except for this code - https://github.com/cursey/safetyhook/blob/main/include/safetyhook/utility.hpp#L17-L26

It seems like concept expressions and auto arguments are both C++20 features. I'm not sure if there's goals to target C++17 or lower (granted, you will have to use polyfill mode anyway for C++20). See: #76

Edit: See reply below.

@cursey
Copy link
Owner

cursey commented Dec 18, 2024

Everything compiles fine except for this code - https://github.com/cursey/safetyhook/blob/main/include/safetyhook/utility.hpp#L17-L26

I'm fine if you want to throw together a PR that simplifies the offending code to use C++17 features. If it's at least mostly equivalent in functionality I would be fine adopting it.

@angelfor3v3r
Copy link
Collaborator

Everything compiles fine except for this code - https://github.com/cursey/safetyhook/blob/main/include/safetyhook/utility.hpp#L17-L26

Can you give this a try?: #83

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants