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

Added marshalling to struct... #525

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

genuinelucifer
Copy link
Contributor

Mostly done... But, one other error is exposed due to this. And moreover it now also marshals few C++ classes to C# struct.
Was that intended while using @class.IsPOD @tritao @ddobrev

Also, I did not add the check for is declared as struct this time.

cls => cls.Bases.Any(clss => clss.IsClass && clss.Class == @class))))
return false;

if (@class.IsPOD || @class.IsValueType)
Copy link
Contributor

Choose a reason for hiding this comment

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

What @tritao had in mind was to use IsPOD instead of IsValueType. So you'd best change it to just:

return @class.IsPOD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@ddobrev
Copy link
Contributor

ddobrev commented Jul 26, 2015

Marshalling some classes as structs is OK, this is what @tritao 's idea is: marshal types based on their usage, not on the key-word which means almost nothing in C++. The error you get, however, is indeed a problem. What exactly is it?

@genuinelucifer
Copy link
Contributor Author

@ddobrev The code marshals a class which was intended to be a Ref type as I had written it for testing a fix when CS_OUT was applied to a ref type argument.
Now it is value type and it is not being properly initialising before use.

@genuinelucifer
Copy link
Contributor Author

@ddobrev Also i wanted to ask about the case when a class in completely different assembly inherits from a class which was previously marshalled as Value Type.
I described the problem on #500 a few days back,

@ddobrev ddobrev force-pushed the master branch 4 times, most recently from d45e2da to dd941d9 Compare August 4, 2015 11:20
@genuinelucifer
Copy link
Contributor Author

@ddobrev Adding this pass to generator didn't seem to be good. Because of two main reasons:
1. If user has 1 class in one project inheriting from a second class in another project. Then if he uses CppSharp on the two projects separately (As in Namespace Test) then it causes error if the Base has laready been marshalled to value type.
2. Some classes are used in Tests like they are ref type and after marshalling them to value type the tests break.
.
For (1) I have introduced a new macro CS_REF_TYPE which will allow the user to force marshalling to ref type even when CheckClassAsValueTypePass is used just on the condition that the CheckMacroPass is used before this pass.

@genuinelucifer
Copy link
Contributor Author

@tritao @ddobrev Please review this.

@genuinelucifer
Copy link
Contributor Author

@ddobrev @tritao Any comments?!

@ddobrev
Copy link
Contributor

ddobrev commented Sep 4, 2015

Hello @genuinelucifer . Please accept my apologies for this huge delay. I've been really busy with Qt#, I am almost ready to build GUI examples with it and it needs a last effort. Meanwhile please proceed with the task about delegates, it's more important than this one anyway.

@genuinelucifer
Copy link
Contributor Author

@ddobrev That's great! :) I had a look into Qt# and it really seems that you have put much much effort into it. Best of Luck!

I'm actually unable to proceed with the delegates task because I actually don't know much about them and actually couldn't figure out how to do that. I will post my real problems on that issue soon.

@ddobrev
Copy link
Contributor

ddobrev commented Sep 9, 2015

@genuinelucifer I am glad you have started thinking about the task. Feel free to ask any questions. I still don't have time to review this one but I have more than enough for questions.

@ddobrev ddobrev force-pushed the master branch 2 times, most recently from ad9811b to beabb82 Compare September 10, 2015 22:38
@tritao tritao force-pushed the master branch 2 times, most recently from ff30267 to e6a74df Compare September 27, 2015 21:58
@tritao
Copy link
Collaborator

tritao commented Oct 2, 2015

Hey @ddobrev we should review this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants