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

Silence or fix some warnings #7562

Closed
wants to merge 1 commit into from
Closed

Conversation

Therzok
Copy link
Contributor

@Therzok Therzok commented Dec 8, 2019

No description provided.

SetValue (value, originator.ObserverToken);
#pragma warning restore CS0618
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs review / comments, it hides something that should not happen.
IOW we should not depend on [Obsolete] API since we generally plan not to have them in XAMCORE_4_0. EIf a newer/better API exists then we should be using it (or we should comment why we can't)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The obsolete method points to this method.

Copy link
Contributor

@spouliot spouliot Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The obsolete method points to this method.

yes, that's exactly my point :) something seems fishy and it needs to be reviewed (and if that's the right approach, some comments needs to be added)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing this properly would probably mean modifying the generator so we generate 2 overloads, one for AUParameterObserverToken and one for the IntPtr version (compat one).

How would one modify the generator so it generates two methods and also forward AUParameterObserverToken to AUParameterObserverToken.ObserverToken

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the non-disable warning fixes to a different PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once Sebastien's concerns are addressed.

Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, after @spouliot concerns are solved.

@mandel-macaque mandel-macaque added the do-not-merge Do not merge this pull request label Dec 23, 2019
@mandel-macaque
Copy link
Member

Adding do not merge until we have answers to the questions from @spouliot

@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 86 tests passed.

Failed tests

  • monotouch-test/watchOS 32-bits - simulator/Debug: Crashed

@Therzok
Copy link
Contributor Author

Therzok commented Dec 23, 2019

Need a response on #7562 (comment)

@Therzok
Copy link
Contributor Author

Therzok commented Jan 30, 2020

Still need a response for the generator question.

@chamons
Copy link
Contributor

chamons commented Jan 30, 2020

Fixing this properly would probably mean modifying the generator so we generate 2 overloads, one for AUParameterObserverToken and one for the IntPtr version (compat one).

How would one modify the generator so it generates two methods and also forward AUParameterObserverToken to AUParameterObserverToken.ObserverToken

If fixing this requires generator changes, I'd consider writing up an issue and letting @dalexsoto or someone else bang that out outside of this PR.

Unless that is you are motivated to do a deep dive in some 🐉 filled code :)

@chamons chamons mentioned this pull request Jan 31, 2020
44 tasks
@Therzok
Copy link
Contributor Author

Therzok commented Jan 31, 2020

Added an item to the list here as per @dalexsoto 's recommendation. Closing this PR.

@Therzok Therzok closed this Jan 31, 2020
@Therzok Therzok deleted the fix-warnings branch January 31, 2020 13:56
@rolfbjarne rolfbjarne added the not-notes-worthy Ignore for release notes label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Do not merge this pull request not-notes-worthy Ignore for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants