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

avoid some const_cast usage #4344

Merged
merged 1 commit into from
Aug 16, 2024
Merged

avoid some const_cast usage #4344

merged 1 commit into from
Aug 16, 2024

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Aug 7, 2022

No description provided.

@danmar
Copy link
Owner

danmar commented Aug 7, 2022

doesn't static_cast mean that the type might change so it's less safe?

@firewave
Copy link
Collaborator Author

firewave commented Aug 7, 2022

doesn't static_cast mean that the type might change so it's less safe?

I don't know. I was surprised that const_cast<const T*> is actually working.

@danmar
Copy link
Owner

danmar commented Aug 7, 2022

I do not feel our code is super elegant. An alternative would be to create a common private const function that is called both from the const and non-const method. We don't need to explicitly cast at all then.

@firewave
Copy link
Collaborator Author

firewave commented Aug 7, 2022

I do not feel our code is super elegant. An alternative would be to create a common private const function that is called both from the const and non-const method. We don't need to explicitly cast at all then.

I am looking into this (also as preparation for misc-const-correctness) and in some cases the non-const pointers might not be necessary at all which might clean up some things.

@danmar
Copy link
Owner

danmar commented Aug 21, 2022

I am looking into this (also as preparation for misc-const-correctness) and in some cases the non-const pointers might not be necessary at all which might clean up some things.

ok I close this PR for now.. but feel free to clean up the code..

@danmar danmar closed this Aug 21, 2022
@firewave firewave reopened this Aug 30, 2022
@firewave firewave marked this pull request as draft August 30, 2022 09:08
@firewave
Copy link
Collaborator Author

ok I close this PR for now.. but feel free to clean up the code..

This would require introducing additional private *Internal() members which would not clean up the code much.

This PR still gets rid of unnecessary/incorrected const_cast calls though.

@firewave firewave marked this pull request as ready for review December 11, 2022 20:57
@danmar
Copy link
Owner

danmar commented Dec 18, 2022

I feel that static_cast is even worse than const_cast though. const_cast is more strict.

@firewave
Copy link
Collaborator Author

const_cast is for casting away const. In this case we are just changing the type without touching the const thus making it unnecessary.

@danmar
Copy link
Owner

danmar commented Dec 18, 2022

const_cast is for casting away const. In this case we are just changing the type without touching the const thus making it unnecessary.

If the casts would change the type then const_cast would generate a compiler error right? So it seems it just casts away the constness.

@firewave
Copy link
Collaborator Author

firewave commented Dec 18, 2022

If the casts would change the type then const_cast would generate a compiler error right? So it seems it just casts away the constness.

I assumed that const_cast is only to get rid of const and not more restrictive than other casts. Need to do some tests with that.

@firewave firewave marked this pull request as draft May 3, 2023 15:42
@firewave firewave mentioned this pull request Sep 28, 2023
@chrchr-github
Copy link
Collaborator

chrchr-github commented Sep 28, 2023

Aren't those instances the canonical use case for const_cast, i.e. resolving between const/non-const overloads?

@firewave
Copy link
Collaborator Author

firewave commented Dec 2, 2023

Partially superseded by #5720. Still lacks the SymbolDatabase adjustments. Even after that I would like to keep it open until the static_cast question has been answered.

@firewave
Copy link
Collaborator Author

firewave commented Dec 2, 2023

#5720 also includes SymbolDatabase now - so this PR is complete obsolete.

@firewave firewave marked this pull request as ready for review July 29, 2024 06:44
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

looks good to me.

@firewave firewave merged commit 5be8d9d into danmar:main Aug 16, 2024
63 checks passed
@firewave firewave deleted the const-cast branch August 16, 2024 11:20
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.

3 participants