search

banditcpp / bandit

Human-friendly unit testing for C++11
https://banditcpp.github.io/bandit/
Other
259 stars 37 forks source link

Fix old style cast warnings #136

Closed MartinDelille closed 5 years ago

MartinDelille commented 5 years ago

Your code was triggering old-style-cast warning. Would you accept this fix?

codecov-io commented 5 years ago

Codecov Report

Merging #136 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #136   +/-   ##
=======================================
  Coverage   98.64%   98.64%           
=======================================
  Files          34       34           
  Lines         956      956           
=======================================
  Hits          943      943           
  Misses         13       13

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 bfdb8a3...5350809. Read the comment docs.

sbeyer commented 5 years ago

Thank you very much. I checked your changes and they are fine. Note that the code you've changed is the external header-only Lean Mean C++ Option Parser library. You might want to submit your changes there, too.

MartinDelille commented 5 years ago

You're welcome!

Unfortunately, I didn't find an easy way to contribute to optionparser.

@mbenkmann You seems to be the author. Would you mind to mirror it to github?

optionparser could become a git submodule then.

mbenkmann commented 5 years ago

The easy way to contribute to optionparser is an email to the mail address listed on the front page

http://optionparser.sourceforge.net/

Given that I have the script all set up to push new releases to SourceForge, including uploading the doxygen docs, I feel no particular desire to mirror it on github. Just check the file into your repository.

As for the change itself: New style casts are ugly.

MartinDelille commented 5 years ago

@mbenkmann Ok if you won't accept the change I might not push them. Tell me if you have a better idea to fix the warning in a nicer way!

mbenkmann commented 5 years ago

I'm sure the compiler you use has #pragmas to disable certain warnings for individual files.

MartinDelille commented 5 years ago

That is not what I call a nice way in my opinion. Let's keep it like that!

sbeyer commented 5 years ago

Oh, it's funny that you discuss it in this merged pull request. ;-)

@MartinDelille I will definitely keep the new version using explicit casts. Often I prefer type(var) casts (i.e., calling the conversion constructor), however, in this case the explicit casts are much better since, for example, const_cast show what part of the cast is currently important (the const-ness change), and reinterpret_cast for a pointer-to-unsigned-long-long cast seems also much more clearer to me.

By the way, for the next major release I have to replace the lean mean option parser by something different, which allows registering new options after the initialization. I want that bandit users can define their own options (see #92), with use-cases such as --fast-tests-only or --intense-testing or --seed=12345 or whatever the user might think of. While the lean mean option parser is great at what it is written for, it does not directly offer such flexibility. (This is an old item on my todo list but currently other things have higher priority, like finishing my PhD.)