You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "mmartell (GitHub)" <gi...@apache.org> on 2020/03/10 22:12:29 UTC
[GitHub] [geode-native] mmartell opened pull request #581:
GEODE-7868: Throw IllegalStateException for unsupported .NET types
The ReflectionBasedAutoSerializer for .NET was causing applications to crash when they used unsupported types. For example, doing a put of an object with a USHORT field causes a StackOverflow exception. With this fix, unsupported types will throw an IllegalStateException.
Also, this fix helped us to understand how GUIDs were only partially working. Although they were being serialized correctly as objects, they were throwing IllegalStateException because they do not have a default constructor.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Another public interface change...
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Throw IllegalStateException for unsupported .NET types
Posted by "mmartell (GitHub)" <gi...@apache.org>.
No. The intent is merely to add support for currently unsupported types in .NET. Will make sure that I don't change any existing APIs. Just waiting on my superfly to complete. Hopefully will close this PR and open a new one today.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on issue #581: GEODE-7868:
Throw IllegalStateException for unsupported .NET types
Posted by "mmartell (GitHub)" <gi...@apache.org>.
This fix isn't quite good enough. It properly throws an exception for System.Guid. However it causes the existing ThinClientPdxTests1 to fail due to lists and maps of System.Objects. The auto serializer needs to handle these as System.Objects instead of throwing an exception. Still investigating whether we can deserialize objects without a default ctor (like guid) using Jake's suggestion.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Log::Debug is our standard in C++/CLI. Don't see a reason to keep these though. Will delete.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
ABI break has been fixed by moving new enums to the end of the list.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on issue #581:
GEODE-7868: Throw IllegalStateException for unsupported .NET types
Posted by lg...@gitbox.apache.org.
This pull request **introduces 5 alerts** when merging cc0d6df9f965649e30966bf20fe4b107d9766773 into 9b145cf60eef6cd7cd0e399d18854d45736f9f0c - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-7bad3516894f33eb3318cea48ea8c95f9b109fc1)
**new alerts:**
* 3 for Useless assignment to local variable
* 1 for Property value is not used when setting a property
* 1 for Container contents are never accessed
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on issue #581:
GEODE-7868: Throw IllegalStateException for unsupported .NET types
Posted by lg...@gitbox.apache.org.
This pull request **introduces 3 alerts** when merging a53758a1c4d809af5339475aba3e9d8d4f872cc4 into 9b145cf60eef6cd7cd0e399d18854d45736f9f0c - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-50b4f64e5681cff9b148c954d610d90e6c2f61c0)
**new alerts:**
* 1 for Property value is not used when setting a property
* 1 for Container contents are never accessed
* 1 for Useless assignment to local variable
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on issue #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by lg...@gitbox.apache.org.
This pull request **fixes 7 alerts** when merging ce7695a4d98afefd88aae2a9651c8dadf52193f9 into bc404f32ac5732bb3922be7cebd713c8f768b24b - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-a170ad966667c744d7199cbfee84602193f88605)
**fixed alerts:**
* 5 for Useless assignment to local variable
* 2 for Poor error handling: empty catch block
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
What's the full story on 64-bit unsigned? Is there no support in PDX for these, or is this UlongArray thing our uint64 support? Is "Ulong" in this context fixed size, or does it vary? In other words, is it just a really unfortunate naming choice, or does Ulong convey meaning?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Just a nitpick here - I personally prefer the single-entry, single-exit paradigm for functions that return a value. I'd declare retVal above the 'if' as = nullptr, then assign into it if len >= 0, eliminate the 'else', and always return retVal. Also what the heck does it mean to have an array of 0 length here, as opposed to just nullptr? Is it even possible to get a negative value for length? That seems incredibly strange, so what does it mean?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
This exercise has indeed uncovered a few API docs bugs. I'll be submitting a ticket for that. You might be right about breaking the ABI due to adding new methods to the interface. I'll test that.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on issue #581:
GEODE-7868: Throw IllegalStateException for unsupported .NET types
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
What are the implications of this fix for compatibility with prior versions? For instance, you mention GUIDs were only partially working - were they working enough to be useful? And do they now no longer work at all?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
What the heck is `Char` in this context? 8-bit? 16-bit? Variable size? Signed or unsigned?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Good catch. Can we just add the new enums at the end of the list?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Yah, the full story. Some consider ulong (same as ULONG), which is 64 bits in .NET, to add confusion. I actually like it. It's C++'s long long and unsigned long long that I dislike. Where Microsoft screwed up is making long and unsigned long only 32 bits in Windows: "... The true "war" was for sizeof(long), where Microsoft decided for sizeof(long) == 4 (LLP64) while nearly everyone else decided for sizeof(long) == 8 (LP64)."
There are no unsigned types in Java. Period. So there was no unsigned support for them in Pdx. This PR fixes that. Not just for the primitives, but for the arrays of the primitives.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
I bet these Log calls were commented out originally for a reason. If we really do want them to stay in the code, can we standardize to the LOGDEBUG macro? Is that not a thing in C++/CLI code? If we don't want 'em, please delete, rather than commenting them out again.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Same comments about interfaces apply here. IPdxReader is also a part of the public API IIRC, so we're probably breaking ABI compat.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Tabs, holy cow. Might be worth just checking the CLI code generally for these, it seems pretty rampant.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Why shorten the names of these? This seems unnecessary....
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Thank you for adding explicit size remarks in the comments!!!
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Not a pain. Your diligence is appreciated.
Fixed.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#581: GEODE-7868: Throw IllegalStateException for unsupported .NET types
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Does this check fail for primitive types then? What about the object version of the primitive types, won’t that pass this test and still cause issues?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Is there a compelling reason we changed this name? UInt64 is, IMO, strongly preferable to ULong....
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Fixed.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on issue #581: GEODE-7868:
Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Changed the title of this PR to better describe the functionality enhancements it provides.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
This is a humongous compatibility break. The only way to make enums properly compatible is to explicitly assign sequential values to the originals, then add the new values in along with explicitly-assigned new values. Otherwise you're going to force a recompile, or introduce all manner of subtle and not-so-subtle bugs into client code.
Also, more tab issues (ugh).
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
What on Earth is going on with the formatting in this switch statement?????? Is this an artifact of viewing it on GitHub, or does it look like this in the IDE as well? Do we need to detabify this file?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Good catch. Looks like the original file had formatting issues. Fixing now.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on issue #581: GEODE-7868:
Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
This PR is being replaced with a new PR. The new PR adds support for unsigned types (and also Decimal and Guid) by just enhancing the ReflectionBasedAutoSerializer. The new PR does not add any new APIs to Pdx.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Done.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on issue #581:
GEODE-7868: Throw IllegalStateException for unsupported .NET types
Posted by lg...@gitbox.apache.org.
This pull request **introduces 3 alerts** when merging 20f609e22c3a36a0f0366c8179dce4cda5c056f5 into 9b145cf60eef6cd7cd0e399d18854d45736f9f0c - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-ba09eaa827d6966715a60cce0d1e9749334cb839)
**new alerts:**
* 1 for Property value is not used when setting a property
* 1 for Container contents are never accessed
* 1 for Useless assignment to local variable
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Same as above. Microsoft uses the C# keywords in their API docs.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Lol, nice change. Always good to do what in the heck you say you're gonna do
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
What's the story behind changing these magic numbers? Was there any significance to their choosing in the first place? They don't appear to correspond to any kind of MAX_* values or things that test edge cases or anything, so why change 'em?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
No, this goes with the new API WriteUnsignedByte.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
These look like MAX_* constants, are there named constants for them anywhere?
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on issue #581:
GEODE-7868: Throw IllegalStateException for unsupported .NET types
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
I think you really have two choices when dealing with unsigned integer. Either treat them a signed in PDX or upscale them to the smaller signed integer. I would suggest treating them as signed in PDX as this is consistent with the C++ serializer.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Adding new member functions to the interface doesn't break the ABI. All Pdx tests still pass (without needing to be rempiled) when run against the new Apache.Geode.dll. This seems consistent with what others are saying. It's only changing function signatures and enums (and a few others) that break the ABI in .NET.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Regarding Java interop with unsigned types, I believe the only issue is if you try to use an unsigned type that exceeds the range of the signed type. For example, 16 bit integers go from -32768 to 32767 for signed, and 0 to 65535 for unsigned. So if you put a value of 50K from C# for example, this shows up as a -4350 in Java. So would be an access violation if you tried to use that as an index into an array in your Java code. As a minimum, we should document this in our Java interop docs (if we have such a thing). I believe the main problem we're trying to solve here is to provide support for the .NET unsigned types. People use unsigned types because they are natural (think size_t), not because they double the range.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Fixed
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
OMG please please _please_ don't ever put single-line if statements without braces in the code!
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on issue #581: GEODE-7868:
Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Just noticed I have a bunch of LGTM alerts. Will look at those. pdxcodemonkey actually mentioned these a few days back.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
This was a good catch. I did have some breaking changes in the API, which have now been fixed. All unsigned types are through new APIs.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
Regarding Java interop with unsigned types, I believe the only issue is if you try to use an unsigned type that exceeds the range of the signed type. For example, 16 bit integers go from -32768 to 32767 for signed, and 0 to 65535 for unsigned. So if you put a value of 50K from C# for example, this shows up as a -4350 in Java. So it would be an access violation if you tried to use that as an index into an array in your Java code. As a minimum, we should document this in our Java interop docs (if we have such a thing). I believe the main problem we're trying to solve here is to provide support for the .NET unsigned types. People use unsigned types because they are natural (think size_t), not because they double the range.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] mmartell commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "mmartell (GitHub)" <gi...@apache.org>.
There are no breaking changes in this PR. Support for new types have been implemented in new APIs.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #581:
GEODE-7868: Support for Unsigned Types in .NET (and Guid)
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Apologies for what is turning into a looong review, but I have a lot of questions that I think we need to answer. I'm not certain we've done our full due-diligence here with respect to backwards compatibility. In particular, I would usually expect to see us define a whole new interface, like say IPdxInstanceFactoryExtended, that inherits from the original and defines the new methods. Otherwise I'm pretty sure we're requiring a recompile here, i.e. making a breaking ABI change.
Also, please check with docs and PM and make sure we don't have further doc obligations beyond the new API doc comments.
[ Full content available at: https://github.com/apache/geode-native/pull/581 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org