Remove unnecessary conditional defines from scenekit.cs - #14503
Conversation
tj-devel709
added
do-not-merge
chamons
left a comment
chamons
left a comment
There was a problem hiding this comment.
See concerns about XAMCORE_3_0
| [Model, Protocol] | ||
| interface SCNProgramDelegate { | ||
| #if MONOMAC | ||
| #if XAMCORE_3_0 |
There was a problem hiding this comment.
I don't believe removing XAMCORE_3_0 is a safe transformation.
There was a problem hiding this comment.
we already have XAMCORE_3_0, right? Since NET == XAMCORE_4_0, The if was just around a NoiOS, so it gives no harm since the method was not there.
There was a problem hiding this comment.
I removed the #if XAMCORE_3_0's because I believe the compiler would not let me add another [NoiOS] outside of it (since it had duplicate attributes) and when I didn't add it, xtro would complain that things were being set for iOS but were not really there. I could be misremembering the above though.
I figured that if I have to add a #if !XAMCORE_3_0 to add the [NoiOS] there as well and the fact that it was already inside a #if MONOMAC and not being able to be called for iOS anyways, that it would maybe be okay to remove.
Any thoughts on this? @mandel-macaque @chamons
There was a problem hiding this comment.
That's some convoluted code... in general removing XAMCORE_3_0 isn't a safe transformation, but in this particular case, the code inside the XAMCORE_3_0 condition didn't do anything, because it was also inside a MONOMAC condition, thus the XAMCORE_3_0 condition is safe to remove.
mandel-macaque
left a comment
mandel-macaque
left a comment
There was a problem hiding this comment.
Looks good to me, but I do agree with @chamons regarding the using being 3 times, move it to a single if.
| #if MONOMAC | ||
| [iOS (8,0)] |
There was a problem hiding this comment.
What was that IOS attr doing there?!?!
| [Model, Protocol] | ||
| interface SCNProgramDelegate { | ||
| #if MONOMAC | ||
| #if XAMCORE_3_0 |
There was a problem hiding this comment.
we already have XAMCORE_3_0, right? Since NET == XAMCORE_4_0, The if was just around a NoiOS, so it gives no harm since the method was not there.
| bool PreserveOriginalTopology { get; set; } | ||
|
|
||
| #if !TVOS && !WATCH | ||
| // note: generator's StrongDictionary does not support No* attributes yet |
There was a problem hiding this comment.
This comment seems important: does the generator's StrongDictionary support No* attributes now? If not, we can't remove the #if conditions (and if it does, then you can remove the comment).
There was a problem hiding this comment.
I would go check the generated code and see what it generates before and after your change.
| [Model, Protocol] | ||
| interface SCNProgramDelegate { | ||
| #if MONOMAC | ||
| #if XAMCORE_3_0 |
There was a problem hiding this comment.
That's some convoluted code... in general removing XAMCORE_3_0 isn't a safe transformation, but in this particular case, the code inside the XAMCORE_3_0 condition didn't do anything, because it was also inside a MONOMAC condition, thus the XAMCORE_3_0 condition is safe to remove.
❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌Tests failed on VSTS: simulator tests iOS. Test results2 tests failed, 106 tests passed.Failed tests
Pipeline on Agent XAMBOT-1030.Monterey |
❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌Tests failed on VSTS: simulator tests iOS. Test results2 tests failed, 106 tests passed.Failed tests
Pipeline on Agent XAMBOT-1017.Monterey' |
5 participants
This PR works to remove the unneeded conditional defines and put in a better input for the generator!