search

OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Cleanup stream read method guards #288

Closed DanRStevens closed 5 years ago

DanRStevens commented 5 years ago

This is the read update corresponding to PR #287. Tagging Issue #278 as related. Tagging PR #281 as related for simplifying the template parameters.

There was one slight difference to the write case. I didn't collapse one of the string read methods into the generalized container form, since the string.data() method didn't have a non-const overload until C++17. Reference: https://en.cppreference.com/w/cpp/string/basic_string/data

The current Linux flags are set for C++14. I'm uncertain if there are blocking issues with the Windows project, particularly regarding the Google Test unit test project. If it's fine to update to C++17, then we can collapse those two methods into one.

Brett208 commented 5 years ago

If PR #290 is merged, this branch can use C++17 features for MSVC builds.

DanRStevens commented 5 years ago

I just took another look at this. It should be fine to merge now.

Once we enable C++17 on all systems we could then remove the one largely redundant method. If we want to do the method removal as part of this PR, then we should wait until the other changes are merged first.

Brett208 commented 5 years ago

Unless we plan to update to requiring C++17 now on all Linux compilers, I would prefer merging and writing up an issue for later work. Better to close this branch out instead of letting it grow stale over time.

We could create a C++17 tag for issues as a reminder on which ones are waiting on C++17 as standard to complete.

DanRStevens commented 5 years ago

Ok, very good suggestion. I wrote up an issue. I'll merge this now.