Proposed-ManagedArray

[MakoEnergy]

This is the updated ManagedArray and converted tests for the Foundation.

[codecov-io]

Codecov Report

Merging #15 into master will decrease coverage by 2.5%. The diff coverage is 96.49%.

@@           Coverage Diff            @@
##           master     #15     +/-   ##
========================================
- Coverage     100%   97.5%   -2.5%     
========================================
  Files           3       4      +1     
  Lines          46     160    +114     
========================================
+ Hits           46     156    +110     
- Misses          0       4      +4

[MakoEnergy]

Leaving notes for what is left to do on this PR. 1) Investigate the Codecov report. I already know small parts of the report are bogus, such as the "at(size_t)" method returning on the ManagedArray. The tests run and pass without throwing, so not hitting the return is simply not possible. I should figure out if the report is fixable.
2) The legit parts of the Codecov report need to be fixed. 3) MSVC is erroring because of warning 4996, which is essentially M$ thinking it knows better than the rest of the coding community and "helping" you, which really just prevents compilation. This can be disabled via macro, but warning suppression is preferred and seems to not be working.

[MakoEnergy]

At this point it appears there is no way to completely appease Codecov and will have to accept it's not perfectly accurate. That, or the compiler is making optimizations that ruin the data on specific tests.

Also, the local linking issue is still occurring on my machine as of the newest updates to the Jagati.

[Sqeaky]

There are a bunch of secondary changes in here that are good.

I like the alphabetizing the file lists in cmake, the Jagati upgrade, the extra test modes in Travis. When I upgrade the Mezz_Test I will do both of these there (I think the file lists are already sorted..

[Sqeaky]

I am reading codecov and the tests. I presume you have seen this page: https://codecov.io/gh/BlackToppStudios/Mezz_Foundation/src/0d0b3d7a3ec053ed7a39a4586a407858737214cf/include/ManagedArray.h ?

I am checking if the tests cover this.

[MakoEnergy]

That's a slightly different page, but yes, I've seen the Codecov results and I refined the tests during the course of this PR specifically to address them. Codecov persists.

[Sqeaky]

Codecov is sometimes a bit behind. I am still reading the test and verify those lines should be covered, but I am really liking this is great.

My only gripe so far is the file enconding. The header files added are Latin1 encoded instead of UTF-8. I am pushing a fix for this now.

[Sqeaky]

I put breakpoints on each of the lines, configured the tests to be built with debug symbols mode by setting CMAKE_BUILD_TYPE to DEBUG. Each of the four break points was hit, but it didn't stop on those lines. It does look like they were optimized out. I am going to try my build again by setting CMAKE_CXX_FLAGS to -O0.

(I know you know how I would do this, but I am trying to get in the habit of writing in a way that will helkp new devs when reading)

[MakoEnergy]

That is really weird. Yes, please fix that. I'll once again double check my settings.

[Sqeaky]

I tried with -Os and -O0 and those lines were "optimized out in both cases" it looks like stuff related to std::function

Some other things I am seeing Near the declaration of ManagedArray::UsedSpace and other data members I see padding warning suppression, this makes sense because we don't know what size the member will be and how that could affect layout in memory. But it does make me wonder how much do we care about alignment here? Are there good reasons to align at least the storage/UsedSpace to sizeof(ElementType)? This is a reasonable default in most cases I think.

[Sqeaky]

Any specific reason in DestroyAtAndAfter you are iterating backwards? I know there no benchmarks on this for this code, but generally things like the cache prefetcher predict going forward and this could defeat them.

[MakoEnergy]

I'm not sure how much we really care about alignment. Like you said, it seemed like a sane default so I went with it.

Iterating backwards in DestroyAtAndAfter (or rather it's equivalent) is how the GCC implementation for Vector does it. I followed suit. I am unaware of any specific reason why they did it that way and it could be re-written if needed/desired, I think.

[Sqeaky]

If you are copying GCCs implementation then there is a good reason to do it that way. Let's not mess around with it. I don't see any alignment stuff in the code now other than the warning suppression.

[Sqeaky]

I got sloppy I can actually get it to hit some of the breakpoints when building with -O0.

I am adding this to the Travis settings and moving the codecov green line to 95% unless you have an issues with that.

[MakoEnergy]

The buffer we place the elements in is an std::aligned_storage.

[MakoEnergy]

Go for it.

[Sqeaky]

Okay, I missed the storage types. I read the iterator types and moved on. I expected to see type decorators for the type at the declaration site, but this is even better.

[Sqeaky]

One other item I found odd is the execution time of the tests. They always take more than 2.5ms on my system. In DEBUG that is more than 3ms. That seems really slow and I didn't see any sleep or timing stuff.

[MakoEnergy]

That is odd. I'm not sure why that would be.