-
Notifications
You must be signed in to change notification settings - Fork 46
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
idl enum generator #105
Comments
Hi @nate800, I do agree with you this is an issue. I am having some trouble with how to fix this without destroying assignability in XTypes, because if you change member names or literal names that check will fail. Some "shadow administration" with fake vs real member names will be necessary. For now I recommend you "just work around it" and wait till either I get around to go through the effort of reworking the name administration or someone else does. |
IIRC, somewhere in the IDL spec it says a leading underscore is used to work around keywords. Something like a leading underscore is stripped off from symbols on input, and prepended to any symbol that clashes with a keyword in the target language. I suppose being masters of the universe, the OMG need not worry about the possibility of a language disallowing a leading underscore and all kinds of other fun things. Or perhaps I have not read the spec carefully enough to have noted that way this transformation is done is language dependent, with the leading underscore simply being the one used for C/C++. |
Hello, @eboasson and @thijsmie , I don't know if I have understood you well. I have tried this IDL code:
When I compile it with IDLC for python I get this code (notice that it is generated without the leading underscore). This code it is not going to work for the None keyword:
When I compile it with IDLC for C I get this code:
If we follow the C philosophy for python, we wouldn't have the problem with the keyword None, we could generate with IDLC for python code like this:
Could this solution be acceptable? |
While this proposition works in your example, there are some problems with it:
I see the real solution as follows:
If those steps seem feasible to go through for you I would be happy to guide you towards making a PR. Join the discord for a chat for some tighter communication loops 😄 |
Hi everybody. We (@Bit-Seq, @jordialexalar and I) have continued working on this issue. We have found a complete solution, it would be a new feature, that it could solve this problem for any target language (C, C++ or Python) and any reserved keyword. The change will be very located, under the scope the IDLC compiler (cyclonedds tool), concretely in the source code file We propose a new flag for IDLC compiler to control the behaviour of this new feature. If this new flag is not set, the keyword check will not be performed (old behaviour is maintained). A new flag
As you can see in the code when this new flag is set, two values are possible for the flag: warning or error.
We have already implemented a prototype of this new function, for the problem of using the reserved keyword None. It has been integrated with the actual IDLC compiler in order to verify that this solution is doable, and the results have been successful. The reserved keyword None is detected during the IDLC conversion, and a warning or error can be displayed to the developer. The change is very located and isolated. No risks have been identified, and it will have a low and depreciable impact in the performance for IDLC compilation process. Our question is, should be see this as a “bug-fix” and use 105 as change-request? Or would it better to add a (new) “enhancement” issue and link them? Any feedback on the preliminary options-names for the flags are welcome. More details will be added to the cyclonedds-docs. Please, @thijsmie and @eboasson or someone else who can guide us, what do you think about the new feature that we are proposing to implement? |
Let me start by saying I really appreciate the effort you are putting into this, this is a real issue that needs fixing which at the moment we do not have time for. That is why I feel a little awkward in saying that no, what you propose is not a good solution of the actual problem. Let me go through why I think that in a couple of notes, and why I stand by my original idea where we implement a solution for the Python backend.
I hope this doesn't come over as harsh. I don't want to discourage you from contributing! I do hope that I've made my thought process clear and why I think we need to solve this on the Python end by the strategy I outlined in my previous comment. |
Hello @thijsmie. Ok, we understand your point of view. We have been checking the IDLC compilation for C++. For an IDL with this code:
We have seen that for any variable the suffix Also, if the name of the identifier is a C++ reserved keyword (in the above example, So, we obtain the following C++ code after IDLC compilation:
This C++ code compile with no problems. So, we think that for C++ the issue 105 is solved. On the other hand, for C, we have seen that this behaviour is not followed. With the same IDL we have generated C code with the IDLC compiler, and we have gotten this C code:
We don't see the suffix We don't see either the suffix So, we have two questions:
Thanks and regards |
Indeed for C this issue is not solved, and it should be! I think we're all so used to C keywords in IDL land that we never needed it implemented. |
Hello @thijsmie, We see possible problems of interoperability between different languages if we implement this solution, maybe we are wrong and need more information. For instance, if we define an IDL like this:
For C++ the IDLC compiler generates a C++ code where the identifier break is translated into For Python, when in the next future we implement the change, the IDLC compiler will generate a Python code where the identifier break is translated into We have done manually this Python translation. The generated Python code has been manually modified, the identifier's name has been changed to We have done another test, where the IDL has a C++ reserved keyword (in this case For this test, both, C++ and Python subscribers, receive the message sent by the C++ publisher. So, we think that in some point, we don't know where, in the cyclonedds-cxx code, the identifier We guess that we have to follow the same logic for Python. Working from the point of view of the programmer, with identifiers like Are we right? |
Yep, you are fully right. That is why the generated classes need an option to retain the original member/type name. The implementation here functions as IDL annotations (see |
Hello @thijsmie. We have this proposal about decorators. Imagine that we have the following IDL
This IDL will be translated into Python with the following decorators (notice the new decorator
Moreover, we will have to create the following function in the file cyclonedds/idl/annotations.py to attend decorator execution
Do we go by the right direction? |
This is indeed in the right direction, however I propose you don't use the class decorators in this case but the member modifiers. Since they might be used for more than just 'keywords' a more generic 'member_name' would be better. Next, I think a plain
|
Hello @thijsmie, While we were making the changes to achieve use Python keywords in the IDL we think we have encountered an interoperability problem. When we publish a message in C++ and another user working in Python subscribes to that topic, the subscriber receives messages. But, when the publisher is the Python user and the subscriber is the C++ user, the subscriber is not able to receive any message. We have verified that the problem persists with the main branch:
Is this an error and we should open an issue? Regards. |
The careful consideration of names of members with keywords is part of this issue, let's get the solution @javiersorianoruiz is working on merged and then re-review the interoperability. |
I am working with @javiersorianoruiz and as I said the interoperability problem was happening before any change from our side, that is, the problem is happening with the main branch. We will do what you say, we will focus on solving members names with keywords. |
Possibly the C++ branch is not careful enough in retaining the original member name in the XTypes typeobject. This will need some separate research. You can check by:
If that is indeed broken an issue can be submitted to the cxx branch. |
We have done this test, a cyclonedds-cxx publisher and other cyclonedds-cxx subscriber using a c++ reserved keywords.
We have successfully checked that subscriber receives the message. So, we think cyclonedds-cxx is correctly managing the use in the IDL of c++ reserved keywords. We don't know how to check the second point that you indicate us in the previous comment:
But we think cyclonedds-cxx is working properly. Even with a cyclone version just downloaded and not using any keyword named field, the problem we have seen it is about interoperability between a cyclonedds-python publisher and cyclonedds-cxx subscriber. In the other direction it works well (cxx -> python). We are using an IDL without any keywords:
To verify all the possibilities, we have also tested :
So it seems is only not working with cyclonedds-python publisher and cyclonedds-cxx subscriber. |
Are you sure it is not the same issue as earlier with the fix posted here: eclipse-cyclonedds/cyclonedds-cxx#331 ? |
This was solved by the great work of @javiersorianoruiz and @Bit-Seq! |
The idl generator allows for a python type to be generated from the following definition.
When generating python code, None is a keyword.
The text was updated successfully, but these errors were encountered: