Skip to content

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave

This comment was marked as outdated.

@firewave

This comment was marked as outdated.

@firewave

This comment was marked as outdated.

@firewave

This comment was marked as outdated.

@firewave
Copy link
Collaborator Author

There's std::experimental::propagate_const as pointed out here: llvm/llvm-project#64988 (comment). Looks quite familiar.

@firewave
Copy link
Collaborator Author

We do not have many pointer left in the code so this should be taking a reference now.

@firewave firewave force-pushed the safe_ptr branch 2 times, most recently from 35a740b to d193431 Compare April 14, 2025 10:15
@firewave firewave marked this pull request as ready for review April 14, 2025 10:24
@firewave
Copy link
Collaborator Author

We do not have many pointer left in the code so this should be taking a reference now.

I will tackle references later on - let's start with the pointers which are not supposed to be references yet.

@danmar
Copy link
Owner

danmar commented Apr 15, 2025

I don't really understand what we achieve with this safe_ptr class. Does it prevent some lifetime issue?

@firewave
Copy link
Collaborator Author

firewave commented Apr 15, 2025

From https://en.cppreference.com/w/cpp/experimental/propagate_const:

It treats the wrapped pointer as a pointer to const when accessed through a const access path, hence the name. 

@firewave
Copy link
Collaborator Author

The name is misleading though. I think this started as something totally different. Not sure what to call it - constness_ptr, propagate_const_ptr, ptr_wrapper, ...?

@danmar
Copy link
Owner

danmar commented Apr 17, 2025

I feel a bit skeptic about this spontanously. But yeah some name that indicates that const is propagated sounds good. But at the same time it shouldn't be too long.

I wonder how people would feel about a cppcheck checker that tells them about accessing non-const member data using const pointers. Spontanously it seems wrong to do that and I could use that in our selfchecks at least. or do you know a use case when we would want to write such data?

I considered such a checker a long time ago but I dropped the idea for some reason. I don't remember why. But I guess it was simply because of limited time and there are plenty of undefined behavior to discover so I rather wanted to work on that.

@firewave
Copy link
Collaborator Author

I wonder how people would feel about a cppcheck checker that tells them about accessing non-const member data using const pointers. Spontanously it seems wrong to do that and I could use that in our selfchecks at least. or do you know a use case when we would want to write such data?

https://trac.cppcheck.net/ticket/11127

Not really a use case but something which will cause a lot of const to be dropped if this is fully applied (I think that might include references - not sure about this, it has been too long) is the ErrorLogger usage.

On a side note I wonder if there is a simple trick to propagate that a function should not be const. Like adding (void)this; to avoid a function to be static.

@firewave firewave marked this pull request as draft April 17, 2025 07:10
@firewave
Copy link
Collaborator Author

I renamed it constness_ptr. Feels awkward but basically describes what it does.

@firewave firewave changed the title added safe_ptr as pointer wrapper to ensure actual method constness added constness_ptr as pointer wrapper to ensure actual method constness Apr 22, 2025
@firewave firewave marked this pull request as ready for review April 22, 2025 07:12
@danmar
Copy link
Owner

danmar commented May 6, 2025

I renamed it constness_ptr. Feels awkward but basically describes what it does.

I like that name better.

@firewave firewave marked this pull request as draft May 6, 2025 09:09
@firewave
Copy link
Collaborator Author

I implemented it in simplecpp - see danmar/simplecpp#548.

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.

2 participants