Skip to content

Update destructors-cpp.md #4846

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 3 commits into from
Dec 12, 2023
Merged

Update destructors-cpp.md #4846

merged 3 commits into from
Dec 12, 2023

Conversation

shalala66
Copy link
Contributor

Demo in example is almost completely wrong. There are some inconsistencies among separate parts of program code:

The parameter of constructor has to be const qualifier. "string" is also called as "character constants", "string constant" and "anonymous string" in C++. This is because a "string" in the C++ standard has the "const qualifier". But the situation in C is slightly different. Since the "const keyword" did not exist in C until the "ANSI C 1989" standard, "string literals" don't have a "const qualifier" for such historical reasons. Hence "non-const string" in C is evaluated as "undefined behavior". Because of the "string literal" that sent as argument to constructor in creation of class object is located in memory in the ".rodata section" of the "data segment", but the constructor parameter "pointer" (char *ch) to string even though itself is in the ".rwdata section" of data segment, it "points" to the "first character address", i.e. "base address" of "string" located in the "read-only section" and store this address. That is why must be added the "const qualifier" that indicating it's "pointing" to "const character". So the compiler generates an error - E0289. To resolve this added "const qualifiers" in 2 places.

Class members in private section were initialized with "nullptr" and "zero" respectively.

Since "delete [] _text" deallocates "*_text", but the "_text" pointer continues to point to the memory location (may store several garbage values) which has been released which then should not be used as it evaluated as undefined behaviour. To prevent this issue added necessarry line "_text = nullptr;" inside the destructor. For resolving there is also could be used a smart pointer, but I did not change the POD style.

In that case in which "destructors", swap functions, move operations, and default constructors should never throw, plus from the performance optimization perspective added "noexcept specifiers" for destructor in 2 places that allows the compiler to skip the process of exception handling which leads to faster execution of the program.

Demo in example is almost completely wrong. There are some inconsistencies among separate parts of program code:

1) 
The parameter of constructor has to be const qualifier. "string" is also called as "character constants", "string constant" and "anonymous string" in C++. This is because a "string" in the C++ standard has the "const qualifier". But the situation in C is slightly different. Since the "const keyword" did not exist in C until the "ANSI C 1989" standard, "string literals" don't have a "const qualifier" for such historical reasons. Hence "non-const string" in C is evaluated as "undefined behavior". Because of the "string literal" that sent as argument to constructor in creation of class object is located in memory in the ".rodata section" of the "data segment", but the constructor parameter "pointer" (char *ch) to string even though itself is in the ".rwdata section" of data segment, it "points" to the "first character address", i.e. "base address" of "string" located in the "read-only section" and store this address. That is why must be added the "const qualifier" that indicating it's "pointing" to "const character". So the compiler generates an error - E0289. To resolve this added "const qualifiers" in 2 places.

2)
Class members in private section were initialized with "nullptr" and "zero" respectively. 

3)
Since "delete [] _text" deallocates "*_text", but the "_text" pointer continues to point to the memory location (may store several garbage values) which has been released which then should not be used as it evaluated as undefined behaviour. To prevent this issue added necessarry line "_text = nullptr;" inside the destructor. For resolving there is also could be used a smart pointer, but I did not change the POD style. 

4)
In that case in which "destructors", swap functions, move operations, and default constructors should never throw, plus from the performance optimization perspective added "noexcept specifiers" for destructor in 2 places that allows the compiler to skip the process of exception handling which leads to faster execution of the program.
Copy link
Contributor

@shalala66 : Thanks for your contribution! The author(s) have been notified to review your proposed change.

Copy link
Contributor

Learn Build status updates of commit b1ea2fb:

✅ Validation status: passed

File Status Preview URL Details
docs/cpp/destructors-cpp.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@Court72
Copy link
Contributor

Court72 commented Dec 11, 2023

@TylerMSFT

Can you review the proposed changes?

When the changes are ready for publication, add a #sign-off comment to signal that the PR is ready for the review team to merge.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@Court72 Court72 added the aq-pr-triaged Tracking label for the PR review team label Dec 11, 2023
Remove some things that aren't strictly necessary
Copy link
Contributor

Learn Build status updates of commit 5e29c1a:

✅ Validation status: passed

File Status Preview URL Details
docs/cpp/destructors-cpp.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 5fd0061:

✅ Validation status: passed

File Status Preview URL Details
docs/cpp/destructors-cpp.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@TylerMSFT
Copy link
Collaborator

Hi @shalala66, Thank you for taking the time to contribute to the docs! Really appreciate it.

For some context: in general, these examples are not meant to be production-ready code--though they should compile (so thanks for fixing that). We assume people know how to program, so what we want to do in these examples is demonstrate some small point about the thing being documented in a way that can be understood quickly. We try not to obscure the point with all the normal error handling code people should know to write. For example, this code doesn't check to see if ch == NULL, which would crash this code in multiple places.

destructors are noexcept by default except in a couple situations that don't apply here. So, it's superfluous to add noexcept. You can test that this destructor is noexcept with something like static_assert(noexcept(str.~str()), "destructor isn't noexcept!");
Marking it noexcept gives the false impression that it's necessary to do explicitly do that. We aren't trying to show the cases where you would want to do that, here.

For this example, keeping the size of the text as a member of the class doesn't add any value and just adds an element of complexity. So, I removed it. Setting _text to NULL is good practice, but not necessary because it gets set to NULL if the memory allocation fails. Setting it to NULL in the destructor is unnecessary because the instance is dead at this point.

I'm not arguing that it isn't a good practice to do these things, but that it is unnecessary for the example. We are trying to just point out just the essentials here.

Your changes + mine will be live by tomorrow.

#sign-off

Copy link
Collaborator

@TylerMSFT TylerMSFT left a comment

Choose a reason for hiding this comment

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

approved with changes

@shalala66
Copy link
Contributor Author

shalala66 commented Dec 12, 2023

For some context: in general, these examples are not meant to be production-ready code--though they should compile

No, it should not compile and it's as soon as obvious in first looking at the code even without compile it.

We assume people know how to program, so what we want to do in these examples is demonstrate some small point about the thing being documented in a way that can be understood quickly.

It's not correct assumption from your side. Where is the defined that this place is only for experienced ones? Cuz, this platform where people learn necessary things from. That is why it should not be depend on knowledge of people. If someone appeals to this platform in that case he need to get information from, including demo code snippets in a correct way. You must not consider that only the people with enough knowledge make a visit to this platform. I could even say vice-versa.

For example, this code doesn't check to see if ch == NULL, which would crash this code in multiple places.

Very good & valuable practice that I can't explain the importance of adding in microsoft/wil community.

Setting _text to NULL is good practice, but not necessary because it gets set to NULL if the memory allocation fails. Setting it to NULL in the destructor is unnecessary because the instance is dead at this point.

I completely don't agree with you and It's most necessary in good practice. I can give you some examples exactly related in this part of your code and continue the dialog in this theme. Then this pointer would be a dangling pointer that points to invalid data or to data which is not valid anymore.

@Jak-MS Jak-MS merged commit 803b891 into MicrosoftDocs:main Dec 12, 2023
@orcmid
Copy link

orcmid commented Dec 12, 2023

@TylerMSFT @shalala66

We try not to obscure the point with all the normal error handling code people should know to write. For example, this code doesn't check to see if ch == NULL, which would crash this code in multiple places.

The perfect-programmer presumption is going to be the death of us all.

We're talking about learn.microsoft.com, yes?

In reality, people learn from Internet examples. The over-simplification of code, lacking defensive programming methods, is going to instruct beginners on their way to becoming paid programmers with very bad habits. The "should know to write" will not happen if there are not exemplary examples in the work seen of others.

I have no magic formula for how to keep error-detection from obscuring the point of an example. It is something worth exploring some sort of practices, like keys to notation/style, that evoke occasions of safe/defensive practices without obscuring the purpose of an example.
I don't suggest redoing all the examples in the documentation here. But there needs to be attention to this somewhere.

The C/C++ language histories and design are not so helpful. We can't expect exceptions to be thrown as a way of saving us from writing safe code.

I'll get off my soapbox now. I just want to pay my respect to the concerns of @shalala66

@shalala66
Copy link
Contributor Author

@orcmid , thank You for your attention and for sharing your opinion.

We're talking about learn.microsoft.com, yes?

yes.

@TylerMSFT
Copy link
Collaborator

@shalala66 , you totally misunderstood me: "No, it should not compile and it's as soon as obvious in first looking at the code even without compile" My point was that the code samples should compile, and the fact that this one didn't was a problem I was thanking you for fixing... Also, it's not my code. It's one of over 10,000 topics, many of which predate me by a very long time, that I have to somehow stay on top of, so there's a finite amount of attention I can give to each one.

@shalala66
Copy link
Contributor Author

shalala66 commented Dec 12, 2023

@TylerMSFT, all clear. Thank you.

@TylerMSFT
Copy link
Collaborator

Guys, you provide reasonable feedback. I'll certainly think more about what to do here.

Many of the reference articles were written a long time ago and have been through more than one publishing system and more than one philosophical iteration about how to approach them. Yes, the learn in learn.microsoft.com is about learning. But you've probably noticed that we have different categories of docs, and they have different purposes.

Reference docs are generally about describing APIs. We aren't really trying to teach you how to program with those--we are trying to describe a system as succinctly and accurately as possible. By analogy, they are sorta like the dictionary in the sense that they provide the definition of the 'words' (APIs), but they won't tell you how to talk to your mother :-) We have how-to and tutorial content where the intent is to teach best practices and basics and so on.

However, I do think the point about learning by example and the fear of cut-n-paste programming is important. I'll mull it over during the holiday.

I do appreciate that you care about the docs! Obviously, we want to make them as good as possible.

@TylerMSFT
Copy link
Collaborator

Oh, regarding the pull request being merged but you don't see it on the live site - that usually takes at least a day. Look for it tomorrow in the afternoon.

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

Successfully merging this pull request may close these issues.

5 participants