search

OutpostUniverse / OP2Utility

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

Add stream size checks #270

Closed DanRStevens closed 5 years ago

DanRStevens commented 5 years ago

This addresses Issue #135.

Ideally, some test code should be added here to exercise the overflow checks.

Additional checks were added for Reader. Writer checks were already sufficient. Only commenting was adding for Writer.

Not directly addressed in the issue is if trivially false checks are zero cost at runtime. That may require examining assembly output to determine. It may be true, but hasn't been verified.

DanRStevens commented 5 years ago

I did some checking of assembly output with Clang, to determine if the checks are indeed zero-cost when they're not actually needed. I noticed the string for negative values check appeared in the output assembly listing using default build settings. It didn't seem to be used in the main code path though. Turning on optimizations with -O2, or guarding the if statement using a constexpr-if (and no optimization) caused the string to be omitted from the assembly output.

I'm thinking perhaps constexpr-if guards should be added around some of the checks.

To enable constexpr-if checks, we should switch compiler settings from C++14 to C++17.

Should such checks be added? Should they be added now before merging, or done as a new branch?

Brett208 commented 5 years ago

Our current nuget package for gtest will fail if we switch to C++17 explicitly in the MSVC build. Maybe preprocessor guard it for Linux only until we figure out a different gtest solution or just push an issue for it.

Brett

DanRStevens commented 5 years ago

Ok, I created issue #275 to track possible future work here. I think we can merge what we have now.