Fix serialization buffer type#6927
Conversation
bangerth
left a comment
bangerth
left a comment
There was a problem hiding this comment.
Yes, nice job. Please make the one variable const and then merge yourself!
| uLongf compressed_data_length = compressBound (oss.str().length()); | ||
| std::vector<char *> compressed_data (compressed_data_length); | ||
| int err = compress2 (reinterpret_cast<Bytef *>(&compressed_data[0]), | ||
| const std::string serialized_data = oss.str(); |
There was a problem hiding this comment.
At first I thought it's a bummer that you need to create a copy of the string, but oss.str() returns the string by value, not by reference, so this is actually more efficient. Nice catch!
|
The |
|
Same test as elsewhere. I think it's safe to assume that this patch isn't responsible for the failing test, so please merge once you made the variable |
how can this test require two additional empty lines and succeed in the Jenkins tester?! |
|
I dont know what is happening with the failing test, I cannot reproduce this locally. If I run the test in the docker container everything passes as expected (both on this branch and on main). I have retriggered the tester, lets see if that fixes it.
Unfortunately, I cant. It is an input and output variable of I will wait for the tester before merging. Maybe it will also help to just replace that file with a fresh reference solution, maybe something is weird about the whitespace in the file (although it wouldnt explain why Jenkins doesnt complain). The only difference between Jenkins and Github actions is the operating system and git version that does the initial checkout of the repository (and one of them merges with main, while the other doesnt). |
| uLongf compressed_data_length = compressBound(serialized_data_length); | ||
| std::vector<Bytef> compressed_data(compressed_data_length); | ||
| int err = compress2 (compressed_data.data(), | ||
| &compressed_data_length, |
There was a problem hiding this comment.
Ah, I see. One would have imagined that that's an input argument, but I recognize now that it's in/out. Never mind then.
|
Since the issue isn't just with this PR, something is going on that is the same for all PRs. Did we merge something bad? |
It started around #6928 and #6929, but I cant see a connection. Also the Jenkins tester is fine, while the Github actions tester that builds on the same docker image fails. My suspicion is that something changed in the Github action tester base stack ( I could try opening a PR that reverts the two PRs and check if the tests pass then. |
|
Let's merge here. Feel free to try and see whether reverting the two other PRs fixes things. |
3 participants
So with all the talk about AI tools finding memory issues in existing software, I thought I ask Codex to take a look at ASPECT. This is the first thing it found (I may open a few more PRs, just need to review the cases).
The main issue here is that the
compressed_databuffer was set up asstd::vector<char *>, while it should have beenstd::vector<Bytef>(Bytefis a zlib defined type which is justunsigned char). This usually works, because a pointer is usually equally sized or larger than achar, but it is a wrong type nevertheless. The new code is also easier to read, and avoids repeated calls tostr()which creates copies of the buffer content. The unusual type names are zlib internal typedefs.