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

Fix wrapping of %define and #define expressions containing chars #93

Open
wants to merge 10 commits into
base: matlab
Choose a base branch
from

Conversation

nunoguedelha
Copy link

Hi @jaeandersson , as per our short discussion, here is the PR fixing the wrapping of global defines and enums having char type elements, or being expanded with char type inputs. Before the fix, the chars would lose the quotes and become variables.

Please refer to robotology-dependencies#1 for more details.

jiulongw and others added 10 commits June 5, 2018 04:27
Problem: When enum value contains compound expression with a char
constant, the quotes around char constant is missing in the generated
expression. Example:

enum media_type {
  YUY2 = ((('2' << 24) | ('Y' << 16)) | ('U' << 8)) | 'Y'
};

The generated C# enum becomes:

public enum media_type {
  YUY2 = (((2 << 24)|(Y << 16))|(U << 8))|Y
}

While the correct representation (after this fix) should be:

public enum media_type {
  YUY2 = ((('2' << 24)|('Y' << 16))|('U' << 8))|'Y'
}

Causes: the exprcompound promotes the expression type from char to int
and uses $1.val in the generated expression. However $1.val does not
contain the quotes. Since the type is promoted to int, there's no way to
know there's char component in the compound expression.

Solution: in exprcomound, use $1.rawval if $1.type is T_CHAR or T_WCHAR.
The rawval contains quotes which yield correct expression.
Fix wrapping of %define and #define expressions containing chars
@nunoguedelha
Copy link
Author

@jaeandersson , if I may, I suggest you do a fast-forward merge in case you accept the PR. This way we avoid diverging our matlab branches.
Feel free to contact us if you have questions on the further fixes we have made on top of this one on our matlab branch:
https://github.com/robotology-dependencies/swig/ pull/2
robotology-dependencies@7e61db6

@nunoguedelha
Copy link
Author

CC @traversaro

@wsfulton
Copy link
Collaborator

Who and where from is the original source of these changes? Shouldn't Go and C# pull requests be submitted to SWIG proper?

@nunoguedelha
Copy link
Author

This PR merges the fix swig#781, please refer to robotology-dependencies#1 for more details.

@nunoguedelha
Copy link
Author

Hi @wsfulton , the original fix swig#781 is already in the master branch of the original repo https://github.com/swig/swig, but not on the matlab branch. Please Let us know how we should proceed.
It might be an opportunity to check if we can PR the other changes from jaeandersson:swig:matlab branch into swig:swig:matlab.

@wsfulton
Copy link
Collaborator

That's okay, it wasn't clear to me when I first saw this that they came from swig/swig originally.

@jaeandersson
Copy link
Owner

Thank you for your contribution. I'll check in the next couple of days. The Matlab branch really needs to get the last push for merging with swig proper. I'll make it a priority. My last couple of months have been pretty crazy, but it's starting to calm down now.

@KrisThielemans
Copy link

Can this be closed please? It's obsolete after merging swig/master in #96.

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.

5 participants