Make catchsegv handle Unicode arguments correctly.

[bigfarts]

This fixes #66.

[jrfonseca]

Thanks for doing this! Indeed lack of proper unicode handling has been a tech debt of this project from the start..

Change looks good. Just one request, please move getoptW.[ch] into thirdparty/getoptW/getopW.[ch], to keep a distinction between this project's and thirdparty's code.

[bigfarts]

Done, please take a look!

[bigfarts]

Sorry, I added a bonus change to avoid const_cast when passing lpCommandLine: it shouldn't affect behavior but it makes it look less scary.

[jrfonseca]

Thanks for the updates.

Sorry, I added a bonus change to avoid const_cast when passing lpCommandLine: it shouldn't affect behavior but it makes it look less scary.

This particular commit doesn't look safe, as there's no guarantee std:string's buffers are null terminated unless one invokes .c_str() method. Best to omit this one.

[codecov-commenter]

Codecov Report

Merging #67 (fab39d7) into master (34e06a5) will not change coverage. The diff coverage is 27.27%.

@@           Coverage Diff           @@
##           master      #67   +/-   ##
=======================================
  Coverage   50.69%   50.69%           
=======================================
  Files          15       15           
  Lines        1943     1943           
  Branches      738      739    +1     
=======================================
  Hits          985      985           
  Misses        737      737           
  Partials      221      221           

Impacted Files	Coverage Δ
src/common/debugger.cpp	47.04% <0.00%> (ø)
src/catchsegv/catchsegv.cpp	45.29% <34.28%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 34e06a5...fab39d7. Read the comment docs.

[bigfarts]

Thanks for the updates.

Sorry, I added a bonus change to avoid const_cast when passing lpCommandLine: it shouldn't affect behavior but it makes it look less scary.

This particular commit doesn't look safe, as there's no guarantee std:string's buffers are null terminated unless one invokes .c_str() method. Best to omit this one.

If you're OK with targeting C++11 minimum, the standard guarantees std::string is null-terminated: https://stackoverflow.com/a/6077274

It looks like in CMakeLists.txt set (CMAKE_CXX_STANDARD 17) and set (CXX_STANDARD_REQUIRED ON) are set, so I believe this change should be safe!

[jrfonseca]

If you're OK with targeting C++11 minimum, the standard guarantees std::string is null-terminated: https://stackoverflow.com/a/6077274

I don't interpret it that way, and the discussion there doesn't look conclusive to me. Yes, C++11 may guarantee that s.c_str() and &s[0] point to the same memory, but unless one calls c_str() at least once since the last string modification, there's no guarantee that s[s.length()) is zero. AFAICT, a std::string implementation can defer zeroing the s[s.length()] until c_str() method is invoked, for efficiency. I rather err on the side of caution. Plus the readability improvement is negligible.

[jrfonseca]

Merged. Thanks.