Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix a StackOverflowException.#250

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
den-run-ai merged 2 commits intopythonnet:masterfrommatthid:fix_stackoverflow
Sep 6, 2016

Conversation

matthid
Copy link
Contributor

Currently I added only the test, and I have no ETA when I can provide the fix. If anyone wants to step in, feel free. I noticed this with F# code (as the compiler will generate a lot of nested types when using DUs).

@den-run-ai
Copy link
Contributor

@matthid is this associated with any particular known issue?

@matthid
Copy link
ContributorAuthor

@denfromufa I did not find anything

@matthid
Copy link
ContributorAuthor

Note for someone hit by this: In my situation a workaround was to mark the class asinternal, as there was no need for it to be exposed...

@matthid
Copy link
ContributorAuthor

Not sure if its the best fix as there now we potentially have uninitialized instances floating around at the place I marked with a comment.

However a "better" fix would need to consider this (edge-)case from the start and would need drastic changes...

@matthidmatthid changed the titleWIP try to fix a stackoverflowexception.Fix a StackOverflowException.Aug 26, 2016
@den-run-ai
Copy link
Contributor

@matthid can you provide this edge case as a test?

Also I think the test should be formatted something like a node class and its child as subclass, otherwise it is confusing why would people ever need recursive types like this. So the use case is for trees and graphs?

@matthid
Copy link
ContributorAuthor

@denfromufa not sure I understand. I already added a testcase.
I'm wondering why the use case matters.

Anyway I tried to explain that the F# compiler generates classes like these for discriminated unions. And I have from time to time written such constructs myself (for exactly the same concept in C#).

Problem is that I had the stack overflow in a class I was not even using in interop (this is due the fact how the F# compiler organizes code)

Hope that helps.

@matthid
Copy link
ContributorAuthor

To clarify: The "recursive" class was not even used in the interop and I still got this error because it was part of a F# module (which will be translated to a static class)...

@matthid
Copy link
ContributorAuthor

Just let me know if there is still something missing in this PR.
@denfromufa If I should refactor the test classes can you please suggest the names I should use? Thank you

@den-run-ai
Copy link
Contributor

@matthid are there any known side-effects of "...we potentially have uninitialized instances floating around..."?

@matthid
Copy link
ContributorAuthor

@denfromufa Not that I'm aware of. On the other hand this case is tested with the unit test. It's just that it "feels" a little bit unclean.

@den-run-ai
Copy link
Contributor

Ok, then let's check what@tonyroberts@filmor@vmuriart think, and then we should be able to merge!

@filmor
Copy link
Member

From my side this looks fine.

@den-run-aiden-run-ai merged commit5aca5cb intopythonnet:masterSep 6, 2016
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@matthid@den-run-ai@filmor

[8]ページ先頭

©2009-2025 Movatter.jp