Skip to content

Clean up and add example for C.32 - raw pointers #1909

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

Merged
merged 10 commits into from
Jul 13, 2022
Merged

Conversation

bgloyer
Copy link
Contributor

@bgloyer bgloyer commented May 4, 2022

Added a simple example for C.32: If a class has a raw pointer (T*), consider whether it might be owning

Also removed mentioning of owning references. I think owning referecnce are rare and generally not a problem in legacy code.
R.3 gives an exception for legacy code to have owing raw pointers but R.4 does not for owning references. If we want to keep the owing references C.33 should probably be updated to delete owing references.

???
class LegacyClass
{
Foo* m_owningPtr;
Copy link
Member

Choose a reason for hiding this comment

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

I suspect both LegacyClass and m_owningPtr` run afoul of NL.10

@hsutter
Copy link
Contributor

hsutter commented Jun 13, 2022

Editors call: Owning raw pointers and references are already banned under R.3 and R.4. Sometimes they do get used, so we think owning references should still be mentioned. Otherwise, the PR looks good, if you can please restore the owning references we can approve it.

@cubbimew
Copy link
Member

How about extending the example to something like

foo* owning = new foo; // change to unique_ptr or owner<T*>
bar* observer = &b; // keep
baz& owning_ref = *new baz; // owning reference; banned
xyz& observer_ref = x; // keep

* F.16 ("in" parameters): Move Matrix example to F.20 (return values) (isocpp#1922)

The `Matrix` example and the notes about assignment appear off-topic in rule F.16, as F.16 is specifically about "in" parameters.

With help from Sergey Zubkov.

* SL.io.50 (Avoid `endl`): Mention string streams (isocpp#1920)

Explicitly mentioned string streams as `endl` insertions into string streams do actually occur in the wild.

With help from Sergey Zubkov.

* Extended E.16 to include copy ctor for exception type, closes isocpp#1921

* Fix GitHub Actions build warnings, Marker style should be `*` (isocpp#1925)

* restored reference

* Added references to note

Co-authored-by: Niels Dekker <[email protected]>
Co-authored-by: Herb Sutter <[email protected]>
@bgloyer
Copy link
Contributor Author

bgloyer commented Jun 16, 2022

How about extending the example to something like

foo* owning = new foo; // change to unique_ptr or owner<T*>
bar* observer = &b; // keep
baz& owning_ref = *new baz; // owning reference; banned
xyz& observer_ref = x; // keep

Is it enough just to leave examples for pointers? It is true that when working with legacy code, ownership of member pointers is often not clear and requires some digging. Owning member pointers was a common practice in older C++. That is not true for references. Owning references have always been a bad idea. Why would anyone have used an owning T& instead of just T? I'm not sure I have ever seen one in the wild. Even in legacy classes, references should be assumed to be non-owning without second guessing if they are really owned. There are many more owning pointers than references in legacy code - I like examples that reflect what is common.

@bgloyer
Copy link
Contributor Author

bgloyer commented Jul 9, 2022

I merge the latest so the diff's are clean now. Anything more on this one?

@hsutter hsutter merged commit cbf4554 into isocpp:master Jul 13, 2022
@hsutter
Copy link
Contributor

hsutter commented Jul 13, 2022

Editors call: Thanks!

@hsutter hsutter self-assigned this Jul 13, 2022
@bgloyer bgloyer deleted the C_32 branch July 16, 2022 02:50
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