String Compression - resolve MSVC warnings for stack memory allocation. #5503

MrSach · 2023-05-24T14:26:29Z

Changed the variable Compresion::Result::Storage of type std::variant to only use Dynamic for memory allocation in ./src/StringCompression.h.

Changed some code that previously defaulted to assign data to the stack in ./src/StringCompression.cpp.

Initialized BytesWrittenOut after MSVC threw another warning:

Warning C6001 Using uninitialized memory 'BytesWrittenOut.'

in ./src/StringCompression.cpp.

This PR should resolve issue #5495.

MrSach · 2023-05-24T14:35:48Z

Draft pull request has been created and is ready for review.
Thank you for your time and feedback.

madmaxoft · 2023-05-25T08:15:13Z

src/StringCompression.cpp

@@ -12,7 +12,6 @@



-


Please don't erase empty lines between functions (style: there are to be 5 empty lines between function definitions). Make sure you run the src/CheckBasicStyle.lua script before committing.

Ok, I will check the pre-commit hook.
I am not sure why it did not run but I will run the pre-commit hook manually next time before the next commit.
The deleted line will be re-added to allow for 5 lines between function definitions.

You need to explicitly enable the pre-commit hook on your local machine (and install Lua, if you haven't already). This is a security measure in git, so that foreign repositories can't run arbitrary code on your computer by putting it into a hook. CONTRIBUTING.md has more info on how to do this.

If you need a Lua interpreter for Windows, my LunaPaak project could help you - it's a single executable that you download and run once, it will register itself as Lua interpreter, and comes with several libraries built in, so it's pretty good for scripting.

madmaxoft · 2023-05-25T08:15:58Z

src/StringCompression.cpp


 	auto DynamicCapacity = Result::StaticCapacity * 2;
 	while (true)
 	{
-		size_t BytesWrittenOut;
+		size_t BytesWrittenOut {};


This is more readable, if MSVC really requires initialization:

Suggested change

size_t BytesWrittenOut {};

size_t BytesWrittenOut = 0;

That is useful to know.
I will use that syntax to initialize instead.

madmaxoft · 2023-05-25T08:17:48Z

src/StringCompression.h

+		/** A store allocated on the heap. */
+		/** Note that this remains a variant for now. */
+		std::variant<Dynamic> Storage;


Since there's now only one type, it no longer needs to be a a variant. In fact, it could very well be a std::vector<byte> for simplification. (Note that this will cause a lot of other changes needed)

Yes, this previously needed the std::variant because of needing to accommodate either a std::array<std::byte> or std::unique_ptr<std::byte[]> for type safety.

To change Storage to a std::vectorstd::byte would need some refactoring in ./src/StringCompression.cpp, but it would be good to simplify and to boost performance.

Would this be raised as an additional issue on git after this current issue?

No, I think you should do this in this PR.

madmaxoft · 2023-05-25T08:28:02Z

Hello. Good first try. I think this whole thing has been really over-engineered and could use some simplification. So using a simple std::vector<byte> for the result storage should be sufficient.

Also, it would be great if you added some documentation to the classes and functions, regarding how to use them. I know it's not part of the original task, but since you're already pretty much rewriting the code, it makes sense to document stuff as long as it's fresh in your head.

Please check the contributing guidelines in CONTRIBUTING.md. There are the code style guidelines, as well as some requirements. You need to add yourself to the CONTRIBUTORS file to indicate your agreement with publishing your under our code license.

MrSach · 2023-05-25T09:05:28Z

Thank you for the feedback.

Should I first make the minor revisions requested for restoring the deleted line and changing the initializer for BytesWrittenOut?

Perhaps I can make a pull request for these two changes to address the current issue and then proceed with the additional changes later?

I will review the CONTRIBUTING.md file and run the pre-commit script before making commits.

madmaxoft · 2023-05-25T09:26:05Z

You should just commit the changes to your repo under the same branch, this will update this PR with the new code.

MrSach · 2023-06-05T12:03:58Z

Hello, I just pushed another commit to my remote.
Now I am waiting for GitHub to update.

MrSach · 2023-06-05T13:29:12Z

Looks like the commit has not gone through correctly.
I will check how to undo this commit first and then push a new one.

MrSach · 2023-06-05T15:28:34Z

Commit 12603f1 appears to have gone through correctly - the others made today can be ignored.
Feedback is welcome and if there are any further changes to make then please let me know.

MrSach · 2023-06-05T15:32:40Z

src/StringCompression.cpp

-				return Buffer.get();
-			}
-		}, Storage), Size
+		&(this->Storage.get()->front()),


&(this->Storage.get()->front())
Is this a safe method to use?
I suppose that if Storage is not empty then this would work, but if it is then would this cause issues?

MrSach · 2023-06-05T15:36:14Z

src/StringCompression.h

-		std::variant<Static, Dynamic> Storage;
+		/** A store allocated on the heap.
+		This uses a smart pointer. */
+		std::unique_ptr <std::vector <std::byte>> Storage;


std::unique_ptr <std::vector <std::byte>> Storage;
As suggested, I have used a std::vector<std::byte> here.
To allocate on the heap, this has been declared using a smart pointer, specifically std::unique_ptr.

This should fix the errors thrown by the script in `continuous-integration/jenkins/pr-merge`: > /home/debian/workspace/cuberite_PR-5503/src/StringCompression.cpp:89:5: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move] std::move (std::make_unique <std::vector <std::byte> > (Dynamic.get(), (Dynamic.get() + DynamicCapacity))), ^ /home/debian/workspace/cuberite_PR-5503/src/StringCompression.cpp:89:5: note: remove std::move call here std::move (std::make_unique <std::vector <std::byte> > (Dynamic.get(), (Dynamic.get() + DynamicCapacity))), ^~~~~~~~~~~ ~ /home/debian/workspace/cuberite_PR-5503/src/StringCompression.cpp:205:6: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move] std::move (std::make_unique <std::vector <std::byte> > (Dynamic.get(), (Dynamic.get() + DynamicCapacity))), ^ /home/debian/workspace/cuberite_PR-5503/src/StringCompression.cpp:205:6: note: remove std::move call here std::move (std::make_unique <std::vector <std::byte> > (Dynamic.get(), (Dynamic.get() + DynamicCapacity))), ^~~~~~~~~~~ ~ /home/debian/workspace/cuberite_PR-5503/src/StringCompression.cpp:246:4: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move] std::move (std::make_unique <std::vector <std::byte> > (Dynamic.get(), (Dynamic.get() + UncompressedSize))), ^ /home/debian/workspace/cuberite_PR-5503/src/StringCompression.cpp:246:4: note: remove std::move call here std::move (std::make_unique <std::vector <std::byte> > (Dynamic.get(), (Dynamic.get() + UncompressedSize))), ^~~~~~~~~~~ ~ 3 errors generated.

MrSach · 2023-06-27T09:14:27Z

https://github.com/cuberite/cuberite/blob/12603f14d59d1d8aac26543af1b3540dec717a77/src/StringCompression.cpp#L11C1-L15

Missing line should now have been re-added.

Let me know if there is anything else to refactor/clean up.

Thanks.

Resolve warnings for stack memory

73b2116

madmaxoft requested changes May 25, 2023

View reviewed changes

Merge branch 'cuberite:master' into Issue5495

5561034

MrSach added 2 commits June 5, 2023 15:09

Further changes to the Compression namespace

e7b9bfa

Compression namespace - further changes

12603f1

MrSach commented Jun 5, 2023

View reviewed changes

MrSach added 3 commits June 6, 2023 11:49

Merge branch 'cuberite:master' into Issue5495

1717880

Merge branch 'cuberite:master' into Issue5495

1eb0c8b

MrSach marked this pull request as ready for review June 22, 2023 19:02

MrSach requested a review from madmaxoft June 27, 2023 09:14

12xx12 added the License not accepted Add this tag to PRs that are finished but the user is a first time contributor and still needs to ac label Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

String Compression - resolve MSVC warnings for stack memory allocation. #5503

String Compression - resolve MSVC warnings for stack memory allocation. #5503

MrSach commented May 24, 2023

MrSach commented May 24, 2023 •

edited

madmaxoft May 25, 2023

MrSach May 25, 2023

madmaxoft May 25, 2023 •

edited

madmaxoft May 25, 2023

MrSach May 25, 2023

madmaxoft May 25, 2023

MrSach May 25, 2023

madmaxoft May 25, 2023

madmaxoft commented May 25, 2023

MrSach commented May 25, 2023

madmaxoft commented May 25, 2023

MrSach commented Jun 5, 2023

MrSach commented Jun 5, 2023

MrSach commented Jun 5, 2023

MrSach Jun 5, 2023 •

edited

MrSach Jun 5, 2023 •

edited

MrSach commented Jun 27, 2023

String Compression - resolve MSVC warnings for stack memory allocation. #5503

Are you sure you want to change the base?

String Compression - resolve MSVC warnings for stack memory allocation. #5503

Conversation

MrSach commented May 24, 2023

MrSach commented May 24, 2023 • edited

madmaxoft May 25, 2023

Choose a reason for hiding this comment

MrSach May 25, 2023

Choose a reason for hiding this comment

madmaxoft May 25, 2023 • edited

Choose a reason for hiding this comment

madmaxoft May 25, 2023

Choose a reason for hiding this comment

MrSach May 25, 2023

Choose a reason for hiding this comment

madmaxoft May 25, 2023

Choose a reason for hiding this comment

MrSach May 25, 2023

Choose a reason for hiding this comment

madmaxoft May 25, 2023

Choose a reason for hiding this comment

madmaxoft commented May 25, 2023

MrSach commented May 25, 2023

madmaxoft commented May 25, 2023

MrSach commented Jun 5, 2023

MrSach commented Jun 5, 2023

MrSach commented Jun 5, 2023

MrSach Jun 5, 2023 • edited

Choose a reason for hiding this comment

MrSach Jun 5, 2023 • edited

Choose a reason for hiding this comment

MrSach commented Jun 27, 2023

MrSach commented May 24, 2023 •

edited

madmaxoft May 25, 2023 •

edited

MrSach Jun 5, 2023 •

edited

MrSach Jun 5, 2023 •

edited