FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

added `constness_ptr` as pointer wrapper to ensure actual method constness by firewave · Pull Request #4785 · cppcheck-opensource/cppcheck · GitHub

added constness_ptr as pointer wrapper to ensure actual method constness - #4785

Draft
firewave wants to merge 1 commit into
cppcheck-opensource:mainfrom
firewave:safe_ptr
Draft

added constness_ptr as pointer wrapper to ensure actual method constness#4785
firewave wants to merge 1 commit into
cppcheck-opensource:mainfrom
firewave:safe_ptr

Conversation

Copy link
Copy Markdown
Collaborator

No description provided.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Collaborator Author

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

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

Copy link
Copy Markdown
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 commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

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

firewave commented Apr 15, 2025
edited
Loading

Copy link
Copy Markdown
Collaborator Author

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. 

Copy link
Copy Markdown
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 commented Apr 17, 2025
edited
Loading

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
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 marked this pull request as draft April 17, 2025 07:10

Copy link
Copy Markdown
Collaborator Author

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

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 marked this pull request as ready for review April 22, 2025 07:12

danmar commented May 6, 2025

Copy link
Copy Markdown
Contributor

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

I like that name better.

firewave marked this pull request as draft May 6, 2025 09:09

Copy link
Copy Markdown
Collaborator Author

I implemented it in simplecpp - see cppcheck-opensource/simplecpp#548.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL