search

catchorg / Clara

A simple to use, composable, command line parser for C++ 11 and beyond
Boost Software License 1.0
649 stars 67 forks source link

use correct length in string constructors #17

Closed rioderelfte closed 8 years ago

rioderelfte commented 8 years ago

The string literal was changed without changing the length of the string given to the constructor.

This causes all my Catch tests to fail when built using AddressSanitizer.

philsquared commented 8 years ago

Thanks for this. That was a bit of a schoolboy error on my part! I'll try and merge this this evening - and port over to Catch too (I see your comments there).

lightmare commented 8 years ago

:scream: This is horrendous. I mean both the original code, and this patch only fixing indices while keeping that awfully obfuscated code in place.

@ianmacs showed what these tests actually do in Catch comment: https://github.com/philsquared/Catch/issues/665#issuecomment-224564469 That's what a proper fix would look like.

philsquared commented 8 years ago

On reflection I tend to agree with you, @lightmare! Not sure why I did it that way to start with (I think the fact there were more separators - with the possibility of more still - made a "find" based approach attractive - but the embedded nulls required the fragile string length in there. You're right: horrendous really!)

I could have my cake and eat it by making the string literal a global and using sizeof on it, but I don't think it's worth it here. I'll consider using something like @ianmacs approach.

Thanks for calling me out on it!

lightmare commented 8 years ago

I think the fact there were more separators - with the possibility of more still - made a "find" based approach attractive - but the embedded nulls required the fragile string length in there.

I see. Tip: next time you need to do that kind of "find" including '\0', you can use good ol' strchr ;)

philsquared commented 8 years ago

Hmm... it would work (and is less brittle). Not sure I like it, though.