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

Chapter 4, Section "Copy Semantics", listing 4-27: The correct result is not due to the correct behavior of the copy constructor. #185

Closed
g8151 opened this issue Sep 5, 2020 · 2 comments

Comments

@g8151
Copy link

g8151 commented Sep 5, 2020

First of all, thank you very much for the book @JLospinoso. Amazing work!

Bug information:
Although Listing 4-27 produces the correct result, there is a subtle bug. The bug is at line 49 "SimpleString a{ 20 }; " or at line 45 "x.append_line("This change is lost.")" depending on your interpretation.

Explanation:
Currently when calling foo in main, we cause SimpleString::append_line to run. In there we check to see if the length of the string to append to, plus the string to append, plus 2 is greater than max_size. Currently this always evaluates to true which means we return false on line 30 and never run std::strncpy(buffer + length, x, max_size - length) on line 31. What this mean as a consequence is that when main finishes it prints the correct result because we never got to append anything in "a" initialized on line 49 not because of the function call "foo(a)" on line 50 which causes the copy constructor on line 18 to run and to create a temporary and completely independent SimpleString object (Figure 4-5). To reveal the bug, comment out the copy constructor
to go back to the situation illustrated in Figure 4-4. This should now cause the copy constructor on line 18 to create a temporary SimpleString object with a "buffer" variable that points to the same "buffer" variable contained in "a" (Figure 4-4).
However the variable "buffer" (which is the same for both a and the temporary object in this case ) never gets updated with the string "This change is lost." passed on line 45. That's because the string "This change is lost." is 20 characters long. "a" initialized on line 49 therefore has max_size equal to 20 which is used in the creation of the temporary object. Consequently the expression if(x_len + length + 2 > max_size) on line 29 always return true which cause line 31 to never run.

Two Easy fixes

1.) declare "a" with a default size of 22 e.g SimpleString a{ 22 } or
2.)change the string passed on line 45 to a string that is at most 18 characters long e.g x.append_line("This change is los"); this is just for illustration purposes.

Steps to reproduce the bug:

1.) declare "a" with a default size of 20 e.g SimpleString a{ 20 };
2.) Comment out the copy constructor
3.) build Listing 4-27
4.) Run a debugger on the executable produced by the build

NOTE: for convenience the file boggy_code_debugging_output.txt containing GDB debugging output has been provided; see that line 31 is never called and this is the main reason why we get the correct result after main finishes.

boggy_code_debugging_output.txt

Steps to verify that the bug is fixed
1.) Apply one of the easy bug fixes above
2.) Uncomment the copy constructor
3.) build Listing 4-27
2.) Run a debugger on the executable produced by build

Note: for convenience the file corrected_code_debugging_output.txt containing GDB debugging output has been provided; see that line 31 is now called. And after the copy constructor creates and appends to the temporary and completely independent SimpleString object, a's variable “buffer” is unaffected.

corrected_code_debugging_output.txt

@JLospinoso
Copy link
Owner

Thanks so much @gh1sl1!

@g8151
Copy link
Author

g8151 commented Sep 27, 2020

My pleasure @JLospinoso.

kruschk pushed a commit to kruschk/ccc that referenced this issue Apr 26, 2021
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

No branches or pull requests

2 participants