You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2020/06/16 19:35:53 UTC
[GitHub] [plc4x] chrisdutz opened a new pull request #168: Feature/c code generation tagged unions
chrisdutz opened a new pull request #168:
URL: https://github.com/apache/plc4x/pull/168
This is the new approach using tagged unions instead of separate types for every sub-type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #168:
URL: https://github.com/apache/plc4x/pull/168#discussion_r442300763
##########
File path: build-utils/language-c/src/main/java/org/apache/plc4x/language/c/CLanguageTemplateHelper.java
##########
@@ -436,8 +476,45 @@ private String toExpression(Field field, Term term, Function<Term, String> varia
}
}
- public String toVariableParseExpression(Field field, Term term, Argument[] parserArguments) {
+ public String toVariableParseExpression(ComplexTypeDefinition baseType, Field field, Term term, Argument[] parserArguments) {
VariableLiteral vl = (VariableLiteral) term;
Review comment:
Yeah ... correct ... but I think there's only one place where it's used ...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] ottobackwards commented on pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #168:
URL: https://github.com/apache/plc4x/pull/168#issuecomment-645992571
@chrisdutz that is fair enough. no reason not to keep it like it is
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] ottobackwards commented on a change in pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #168:
URL: https://github.com/apache/plc4x/pull/168#discussion_r442298201
##########
File path: build-utils/language-c/src/main/java/org/apache/plc4x/language/c/CLanguageTemplateHelper.java
##########
@@ -436,8 +476,45 @@ private String toExpression(Field field, Term term, Function<Term, String> varia
}
}
- public String toVariableParseExpression(Field field, Term term, Argument[] parserArguments) {
+ public String toVariableParseExpression(ComplexTypeDefinition baseType, Field field, Term term, Argument[] parserArguments) {
VariableLiteral vl = (VariableLiteral) term;
Review comment:
"CAST" could be a constant. If there are other things you could do that too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] ottobackwards commented on a change in pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #168:
URL: https://github.com/apache/plc4x/pull/168#discussion_r441130521
##########
File path: build-utils/language-c/src/main/java/org/apache/plc4x/language/c/CLanguageTemplateHelper.java
##########
@@ -436,8 +476,45 @@ private String toExpression(Field field, Term term, Function<Term, String> varia
}
}
- public String toVariableParseExpression(Field field, Term term, Argument[] parserArguments) {
+ public String toVariableParseExpression(ComplexTypeDefinition baseType, Field field, Term term, Argument[] parserArguments) {
VariableLiteral vl = (VariableLiteral) term;
Review comment:
magic string
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #168:
URL: https://github.com/apache/plc4x/pull/168#issuecomment-645823603
> that is too much to look through all of it.
> It looks good. A couple of things that come to mind:
>
> * can we run clang-format after generation?
> * should we be generating some logging?
> * _message as a parameter is strange to _me_
> * so many (*_message)-> everywhere, can we use a typedef or something?
>
> None of these things are for this PR
Well I would rather have the code generated in a way that it doesn't require formatting ...
I didn't generate any logging as I didn't know of any logging framework to use ... we could whip up a skeleton of functions that we can use for logging and plug in a "printf" logger or whatever we like to do the actual logging ... that might be a good idea.
As I mentioned before ... I wanted to ensure the internal variable names don't clash with the ones a user can use in mspec ... people tent to get anoyed if they have to use other named due to internal stuff.
Why do we need to typedef that ... It's not that anyone is ever going to write that code ... but if you like, of course that's probably a really simple change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz merged pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
chrisdutz merged pull request #168:
URL: https://github.com/apache/plc4x/pull/168
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #168:
URL: https://github.com/apache/plc4x/pull/168#discussion_r442008506
##########
File path: build-utils/language-c/src/main/java/org/apache/plc4x/language/c/CLanguageTemplateHelper.java
##########
@@ -436,8 +476,45 @@ private String toExpression(Field field, Term term, Function<Term, String> varia
}
}
- public String toVariableParseExpression(Field field, Term term, Argument[] parserArguments) {
+ public String toVariableParseExpression(ComplexTypeDefinition baseType, Field field, Term term, Argument[] parserArguments) {
VariableLiteral vl = (VariableLiteral) term;
Review comment:
??
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #168:
URL: https://github.com/apache/plc4x/pull/168#discussion_r442009007
##########
File path: sandbox/plc4c/generated-sources/modbus/src/modbus_constants.c
##########
@@ -24,18 +24,18 @@
#include "modbus_constants.h"
// Parse function.
-plc4c_return_code plc4c_modbus_read_write_modbus_constants_parse(plc4c_spi_read_buffer* buf, plc4c_modbus_read_write_modbus_constants** message) {
+plc4c_return_code plc4c_modbus_read_write_modbus_constants_parse(plc4c_spi_read_buffer* buf, plc4c_modbus_read_write_modbus_constants** _message) {
Review comment:
Well I had to somehow make sure the names a user can choose in mspec don't interfere with those used internally ... so I prefixed them with a "_" if you have another suggestion, I'm fine with changing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] ottobackwards commented on pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #168:
URL: https://github.com/apache/plc4x/pull/168#issuecomment-645005753
that is too much to look through all of it.
It looks good. A couple of things that come to mind:
- can we run clang-format after generation?
- should we be generating some logging?
- _message as a parameter is strange to _me_
- so many (*_message)-> everywhere, can we use a typedef or something?
None of these things are for this PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] ottobackwards commented on a change in pull request #168: Feature/c code generation tagged unions
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #168:
URL: https://github.com/apache/plc4x/pull/168#discussion_r441131919
##########
File path: sandbox/plc4c/generated-sources/modbus/src/modbus_constants.c
##########
@@ -24,18 +24,18 @@
#include "modbus_constants.h"
// Parse function.
-plc4c_return_code plc4c_modbus_read_write_modbus_constants_parse(plc4c_spi_read_buffer* buf, plc4c_modbus_read_write_modbus_constants** message) {
+plc4c_return_code plc4c_modbus_read_write_modbus_constants_parse(plc4c_spi_read_buffer* buf, plc4c_modbus_read_write_modbus_constants** _message) {
Review comment:
to me, when I see _var_name I think of a private var, not only in python but in some c-ish languages I've worked in. is there some standard that you are writing two using _ in a parameter name?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org