search

mathtone / MIST

Implements change notification for properties (ie: INotifyPropertyChanged) using IL weaving and a custom Visual Studio build task.
MIT License
49 stars 7 forks source link

OnChange notifications crashes for value-types #6

Closed kohnsten closed 8 years ago

kohnsten commented 8 years ago

When the auto-property is a value type, such as bool, int or double the generated IL crashes at run-time.

However, if you wrap the value type in a class that overrides Equals(), the code works. This of course is not possible in production code.

mathtone commented 8 years ago

Confirmed, the OnChange notification isn't working for value types, I shouldn't have missed that but I was anxious to get the build out. In fact it doesn't seem to be working at all, which is strange. I'll sort this out tonight, thanks.

mathtone commented 8 years ago

For some reason all the unit tests pass but it doesn't seem to work from the nuget package. I must have packaged it wrong. Investigating...

aquamoth commented 8 years ago

You havn't pushed any commits regarding this issue so it's hard to help out. It was easy to extend the existing unit test to make it fail, so I'm sure you've come so far.. Let me know if I can help out!

mathtone commented 8 years ago

I was actually able to replicate this, I made a few changes and added new tests but the OnChange notification is still broken. I didn't want to check in broken code but I ran out of time. I'll work on it some more tonight but in the meantime I'll go ahead and check it in. If you want to take a look and see what you can do that would be great. If you take a look at the "OnSet_Mist_Vs_Standard" and "OnChange_Mist_Vs_Standard" yo'll get an idea of the direction I was going in. Good luck!!

aquamoth commented 8 years ago

I've looked at your code and I think I understand what you are trying to do. My main concern with testing if Mist generates the same opcodes as a compiled manual implementation is that the compiler generates different IL for Debug and Release. Thus, your unit tests fail in one mode.

mathtone commented 8 years ago

Yeah that's a good point, but I think I can adjust the tests to take that into account. Implementing this OnChange style notification reliably has been a bit of a challenge.

On Sat, Jun 11, 2016 at 9:28 AM, Mattias Åslund notifications@github.com wrote:

I've looked at your code and I think I understand what you are trying to do. My main concern with testing if Mist generates the same opcodes as a compiled manual implementation is that the compiler generates different IL for Debug and Release. Thus, your unit tests fail in one mode.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mathtone/MIST/issues/6#issuecomment-225361726, or mute the thread https://github.com/notifications/unsubscribe/AHPnKqQXDVmlYqb0Htp3kK_2S_D5M35Mks5qKrfvgaJpZM4Ix74n .

mathtone commented 8 years ago

This should be working now.