search

MolecularMatters / raw_pdb

A C++11 library for reading Microsoft Program DataBase PDB files
BSD 2-Clause "Simplified" License
675 stars 91 forks source link

Fixed snprintf into const char* #51

Closed DRKV333 closed 1 year ago

DRKV333 commented 1 year ago

I've tried compiling the examples with MSVC2022, and it didn't work due to this write to string::data(). I'm not sure if this works elsewhere, but according to the standard, data() returns a const char*. With a const_cast this is still very much undefined behavior, but at least it compiles now, and it seems to work fine.

tromey commented 1 year ago

FWIW I think you can use &result[0] to avoid the const cast.

tivolo commented 1 year ago

Neither the const_cast nor &result[0] should be used.

In pre-C++17, data() only returns immutable storage and changing the underlying data is UB. Since C++17, data() offers a non-const overload which is what is used in this case.

What is the error you're seeing with MSVC2022? Should compile fine when using at least C++17.

DRKV333 commented 1 year ago

It's just the regular "const char is incompatible with char". The project configuration here says we're working with C++11. https://github.com/MolecularMatters/raw_pdb/blob/26683ab3cff61c64f5d4c91254b7696a1797cb0c/CMakeLists.txt#L5 That actually generates a vcxproj for C++14, since C++11 doesn't appear to be an option.

tivolo commented 1 year ago

Either way, the const_cast is not a solution, but a duct tape. This has to be done differently entirely (e.g. snprintf into a stack buffer, return a std::string from there).

DRKV333 commented 1 year ago

Sure, if you want to fix this properly, I'd be more than happy with that too. Though considering that this is just in the example code, I'm not sure it's worth the effort.

tivolo commented 1 year ago

Shouldn't be a lot of work, will do this tomorrow.

MolecularMatters commented 1 year ago

We fixed this in 8c6a7146393c83d27fa101e8bc8017f2a7f151df.