Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor interop to use single type for data access. #57

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ZehMatt
Copy link
Contributor

ZehMatt commented Jan 26, 2018

This should simplify the need of having a specific array object. I also added basic iterators so most STL algorithms should work which will help later replace them with std::array, std::vector and so on without having the need to refactor any code afterwards.

@@ -14,14 +14,14 @@
#define FORCE_ALIGN_ARG_POINTER
#endif

#ifdef COMPAT_STD_BYTE
//#ifdef COMPAT_STD_BYTE

This comment has been minimized.

@AaronVanGeffen

AaronVanGeffen Jan 26, 2018

Member

Should probably not redefine std::byte.

This comment has been minimized.

@ZehMatt

ZehMatt Jan 26, 2018

Author Contributor

Oh right, I meant to leave that as is. This should be probably wrapped with MSC_VER

This comment has been minimized.

@AaronVanGeffen

AaronVanGeffen Jan 26, 2018

Member

I think it's been introduced just for compatibility with macOS. Current versions of GCC and Clang also support std::byte.

@AaronVanGeffen

This comment has been minimized.

Copy link
Member

AaronVanGeffen commented Jan 26, 2018

Nice work, @ZehMatt!

@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/interop branch 3 times, most recently from 39d885e to 350862c Jan 26, 2018

}
loco_global_iterator& operator++(int)
{
++_ptr;

This comment has been minimized.

@jgottula

jgottula Jan 27, 2018

Contributor

No temporary storage of "before" value for post-increment overload? Looks like a mistake.

@jgottula

This comment has been minimized.

Copy link
Contributor

jgottula commented Jan 27, 2018

@ZehMatt I like it. There are a few small issues, though.

I made a branch based off of yours with a couple of postfix operator increment/decrement fixes, as well as a commit to un-break the loco_global template overloads of utility::strcpy_safe and utility::strcat_safe:

ZehMatt/OpenLoco@refactor/interop...jgottula:ZehMatt/refactor/interop

@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/interop branch from 350862c to 172932b Jan 27, 2018

}
loco_global_iterator operator++(int)
{
loco_global_iterator<T> temp = *this;

This comment has been minimized.

@jgottula

jgottula Jan 27, 2018

Contributor

Minor inconsistency: temporary iterator is named temp in operator++, but named tmp in operator--.

loco_global operator++(int)
{
addr<TAddress, T>()++;
return *this;

This comment has been minimized.

@jgottula

jgottula Jan 27, 2018

Contributor

This is just wrong.

Suppose there's a global like this:

loco_global<int32_t, 0x12345678> example;

And suppose that the value of the int32_t at address 0x12345678 is currently 99.

Then someone does this in their code:

int32_t x = example++;

The programmer who writes that code expects x to have the value 99. But x will instead have value 100, because the post-increment operator is currently implemented identically to the pre-increment operator: it increments the value at the 0x12345678 from 99 to 100, and then returns a loco_global for 0x12345678, the value at the address after the increment operation.

The fact that you're returning a copy of the loco_global by-value rather than by-reference doesn't actually accomplish anything, because the original loco_global and a copied old version of the loco_global are not different after an arithmetic operation is done, because the arithmetic operation affects the pointed-to value. The loco_global just stores the address; it knows nothing about the status of the value at that address.

Or in other words: this is a different implementation situation for operator overloading than what you'd do for a smart pointer or an iterator, where the increment or decrement operation affects the smartptr/iterator's address/offset itself, and so you can simply return an old copy of the smartptr/iterator.

So either:

  • (a) loco_global::operator++(int) needs to have return type T, and store a temporary T before doing the increment, and return that temporary T; or
  • (b) if (a) is objectionable due to the fact that you're no longer returning a loco_global wrapper of some kind and are simply returning a value of type T which can't be chained for further operations as a loco_global could be, then the post-increment operator should just be explicitly deleted (void operator++(int) = delete;) and users of loco_global should be therefore forced to use only pre-increment or pre-decrement, to prevent them from making mistakes based on their expectations of how post-increment of a value-wrapper-type like loco_global would work.

Here's an test you can do to show that the current implementation works incorrectly. Put this at the beginning of main:

uint32_t before  =   _srand0;   std::printf("%08X\n", before);
uint32_t preinc  = ++_srand0;   std::printf("%08X\n", preinc);
uint32_t postinc =   _srand0++; std::printf("%08X\n", postinc);
uint32_t after   =   _srand0;   std::printf("%08X\n", after);

Expected output:

00000000
00000001
00000001
00000002

Actual output:

00000000
00000001
00000002
00000002
loco_global operator--(int)
{
addr<TAddress, T>()--;
return *this;

This comment has been minimized.

@jgottula

jgottula Jan 27, 2018

Contributor

loco_global::operator--(int) is wrong in exactly the same manner as loco_global::operator++(int) and should be corrected in the same manner. (See comment above.)

@jgottula

This comment has been minimized.

Copy link
Contributor

jgottula commented Jan 27, 2018

Please cherry-pick 223aee2 onto this branch.

As this branch currently stands, the special overloads of utility::strcpy_safe and utility::strcat_safe intended for loco_global<char[N]> will successfully resolve for loco_global<char*>, and they should not. They were designed with the specific intent to only work with loco_global's that point to a char buffer of known dimension.

Currently the overloads will resolve successfully, and then they will use the loco_global::size() member function (which has ambiguous semantics: it returns array dimension in the local_global<char[N]> case, but size-of-pointer in the loco_global<char*> case) to erroneously determine that the destination C string buffer has space for exactly sizeof(char*) (4, or perhaps 8) characters, which is not what should happen.

return reinterpret_cast<pointer>(&addr<TAddress, type>());
}

reference operator[](size_t idx)

This comment has been minimized.

@jgottula

jgottula Jan 27, 2018

Contributor

loco_global::operator[] needs to take an int parameter (as the old loco_global_array::operator[] implementation did), not a size_t. Otherwise, the compiler gets upset, because the built-in operator[] in the language for arrays uses int as the indexing parameter.

Specifically, you get this when you attempt to use operator[] on a loco_global<T[N]>:

error C2666: 'openloco::interop::loco_global<openloco::industry [128],6047068>::operator []': 2 overloads have similar conversions
note: could be 'openloco::industry &openloco::interop::loco_global<openloco::industry [128],6047068>::operator [](::size_t)'
note: or       'built-in C++ operator[(openloco::industry *, int)'
note: while trying to match the argument list '(openloco::interop::loco_global<openloco::industry [128],6047068>, openloco::industry_id_t)'

Also, when changing the type of idx back to int, the if statement condition for the bounds check should be restored to the way it was before in loco_global_array::operator[]: by first checking for negative values and then explicitly casting to size_t for the comparison with size(), it avoids signed/unsigned comparison problems as well as compiler warnings related thereto.

@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/interop branch from 172932b to 2883812 Jan 28, 2018

@jgottula

This comment has been minimized.

Copy link
Contributor

jgottula commented Jan 28, 2018

#57 (review) looks good now. 👍
#57 (review) looks good now. 👍
#57 (review) looks good now. 👍

loco_global_iterator::operator++(int) and loco_global_iterator::operator--(int) are still not consistent with each other for some reason.

And #57 (comment) still needs to be applied/addressed.

@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/interop branch from 2883812 to 789c54d Jan 29, 2018

@ZehMatt ZehMatt force-pushed the ZehMatt:refactor/interop branch from 7763cd2 to bc397fb Jan 29, 2018

@ZehMatt

This comment has been minimized.

Copy link
Contributor Author

ZehMatt commented Jan 29, 2018

I broke this PR, ignore it.

@jgottula

This comment has been minimized.

Copy link
Contributor

jgottula commented Jan 29, 2018

Superceded by #75.

IntelOrca added a commit that referenced this pull request Feb 5, 2018

Merge pull request #75 from jgottula/ZehMatt/refactor/interop
Refactor interop to use single type for data access. [#57 reborn]

This should simplify the need of having a specific array object. Also adds basic iterators so most STL algorithms work which will help later replace them with std::array, std::vector and so on without having the need to refactor much code afterwards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.