Proposed-StaticAny

[MakoEnergy]

Ported the StaticAny from the monolithic repo.

As a bonus, I removed the redundant "ManagedArray" from its test names.

[Sqeaky]

In StaticAny.h line 330 is a bit long? That is some messy typename aliasing, I don't see how, but could it be two types?

[Sqeaky]

Line 382 as well, and both of those are just remarks.

[Sqeaky]

On the tests in test/StaticAnyTests.h around line 90 you show tests where it fails to cast doubles to floats. Do you have any tests that attempt to cast away consts-ness? That should fail, but what actually happens?

[MakoEnergy]

I pushed a few minor changes based on some of the errors being generated. Should reduce but not eliminate them.

The comments I have removed. As for const-ness...currently the code decays the type immediately before storage. So it discards const-ness.

[Sqeaky]

Interesting on the Const-Decay. So no matter what it stores an actual value internally. Do you think precludes the normal allocation, math and other optimization by elision that const normally enables? Or do we expect to get that back if the StaticAny itself is declared const?

Also do that mean this is safe to modify const values or that it enables dangerous operations?

[MakoEnergy]

It's important to note that std::decay doesn't affect pointers. And given that this will decay the type it will store whatever you give it by value or pointer. It doesn't really bypass const-ness. You are getting a different memory space when you go to extract. If you are super concerned about performance that const gives you, then using a type-erasing container probably isn't the tool you want/need.

[Sqeaky]

This looks good. Let me know if you want another pre-merge review.

[MakoEnergy]

I'm leaving here a few notes on things I've discovered during the course of fixing these bugs.

1) While I can test and verify that the memory access done in the StaticAny is safe, it's not guaranteed by the standard to be safe. In some edge cases it can break due to an overzealous optimizer. The standard fix for this is std::launder, which is made available in C++17. That will make memory accesses like the ones used in the StaticAny completely safe. Considering updating to C++17 in the Jagati.

2) MSVC is bad. Appveyor specifically is using VS 2015, which despite the hint given in it's title, has little/poor C++14 support. At the time of this writing I'm trying to work around an error generated on appveyor (but not Jenkins, as our Jenkins uses VS 2017) because constexpr functions are meant to just a return statement. Considering dropping support for VS 2015.

3) The last major category of bugs with this PR I haven't researched heavily and found other limitations for is an error being generated due to extra padding in the StaticAny. This started out as 5 errors and I've reduced it down to one. I'm not sure how or why it's still occurring. I could wrap the whole class in the padding warning suppression, but I shouldn't have to. Still looking into it.

Edit: Had to wrap the whole class in the warning suppression. Considering adding that warning to the global warning suppression list.

[MakoEnergy]

Given that C++14 is the current standard we are using, and even considering jumping to C++17...AND VS 2015 lacks completely C++14 support...I have decided to drop testing support for VS 2015.

[codecov[bot]]

Codecov Report

Merging #35 into master will increase coverage by 0.56%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   98.09%   98.65%   +0.56%     
==========================================
  Files           6        7       +1     
  Lines         210      298      +88     
==========================================
+ Hits          206      294      +88     
  Misses          4        4

[Sqeaky]

I will review this tonight

[Sqeaky]

What is the Appveyor image? Is that the disk image used in spinning up a VM?

[Sqeaky]

This looks really good.

[MakoEnergy]

It's a disk image of pre-loaded software. The default image (when you don't specify one) doesn't have VS 2017.

[Sqeaky]

Here are the test runs on my system

--= Summary =--
   Success 451
   Warning 0
   Skipped 0
 Cancelled 0

Inconclusive 0 Failed 0 Unknown 0 NotApplicable 0

From 451 tests the worst result is: Success

[Sqeaky]

Sorry, I should have done that