search

freebsd / kyua

Port/Package build and test system
https://github.com/freebsd/kyua/wiki
BSD 3-Clause "New" or "Revised" License
149 stars 42 forks source link

Explicitly require a C++11 capable toolchain when building Kyua #191

Closed ngie-eign closed 5 years ago

ngie-eign commented 5 years ago

While it's true that C++11 isn't strictly required with ATF/Kyua, in order to move forward and leverage new C++ language features, the project needs to require newer C++ standards.

This will also allow Kyua to shed backwards compatibility code for std::shared_ptr, etc, further streamlining the project.

jmmv commented 5 years ago

Have you seen utils::text::regex?

Not saying requiring C++ is a bad idea (in fact, I wanted to do that at some point and then clean up things like utils/shared_ptr.hpp), but I have to question why this would be a prerequisite for googletest support.

ngie-eign commented 5 years ago

Have you seen utils::text::regex?

I hadn't seen that. I was trying to stick to what was provided upstream and I completely missed that... hmmm.

Not saying requiring C++ is a bad idea (in fact, I wanted to do that at some point and then clean up things like utils/shared_ptr.hpp), but I have to question why this would be a prerequisite for googletest support.

I was unaware that the regex support had already been implemented using the C standard libraries, instead of the C++ standard libraries.

Given the number of bugs in FreeBSD's regex parsing logic (in the past), I think it would be better to settle on a C++ standard (since it's probably more standard across different implementations than the C ones).

I can work towards removing unnecessary (post-C++-11) code in another PR.

ngie-eign commented 5 years ago

Do we really need three commits to add this? Looks like a single one would be sufficient...

I was trying to break down the work in terms of individual commits (out of habit), but I can squash them together.

jmmv commented 5 years ago

Breaking stuff into individual, smaller commits, is great -- but stuff should be grouped semantically. And in this case, the three modified files were changed to achieve the same thing. Thanks for merging them.

jmmv commented 5 years ago

By the way, for these little nits, I could just pull your commit, squash the minor edits myself, and then merge the change. I think this would mess the GitHub UI in that it wouldn't show the PR as merged, but if you are OK with it, I can do that to avoid roundtrips.

ngie-eign commented 5 years ago

By the way, for these little nits, I could just pull your commit, squash the minor edits myself, and then merge the change. I think this would mess the GitHub UI in that it wouldn't show the PR as merged, but if you are OK with it, I can do that to avoid roundtrips.

I'm totally fine with that. It should show the merged commits as a joint effort with us both contributing :).

ngie-eign commented 5 years ago

@jmmv: could you please merge this change?

ngie-eign commented 5 years ago

Ugh, the std::regex implementation done by libc++ and libstdc++ are buggy, so the original reason for moving to C++11 is diminished a bit. However, it makes sense to move forward with this change.

jmmv commented 5 years ago

Yep, I think it makes sense to move forward too. It's 2019.

Interested in killing shared_ptr next and thus putting this to use? :)

ngie-eign commented 5 years ago

Yep, I think it makes sense to move forward too. It's 2019.

Interested in killing shared_ptr next and thus putting this to use? :)

Will do :).