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 2021/03/18 12:52:50 UTC
[GitHub] [plc4x] sruehl commented on a change in pull request #230: Feature/string enum mspec
sruehl commented on a change in pull request #230:
URL: https://github.com/apache/plc4x/pull/230#discussion_r596813408
##########
File path: .gitignore
##########
@@ -181,3 +181,14 @@ build-iPhoneSimulator/
/sandbox/plc4go/temp/
/plc4go/.idea/
plc4j/examples/hello-storage-elasticsearch/.factorypath
+
+
+*.make
+*.cmake
+*.internal
+link.txt
+Makefile
Review comment:
```suggestion
```
This would actually remove the plc4go "wrapper" Makefile from plc4gp again so a more specific ignore would be good. (Not sure why the Makefile is ignored here)
##########
File path: build-utils/language-c/pom.xml
##########
@@ -32,6 +32,40 @@
<name>PLC4X: Build Utils: Language: C</name>
<description>Code generation template for generating C code</description>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-invoker-plugin</artifactId>
+ <version>3.1.0</version>
+ <configuration>
+ <debug>true</debug>
+ <projectsDirectory>src/test/resources</projectsDirectory>
+ <cloneProjectsTo>${project.build.directory}/integration-tests</cloneProjectsTo>
+ <!-- Removed so that a local version of the build-tools could be used. Need to add it back in -->
Review comment:
Guess this needs to be done before merging?
##########
File path: build-utils/language-c/src/main/java/org/apache/plc4x/language/c/CLanguageTemplateHelper.java
##########
@@ -288,7 +299,7 @@ public String getTypeSizeForField(TypedField field) {
public String escapeValue(TypeReference typeReference, String valueString) {
if (valueString == null) {
- return null;
+ return "NULL";
Review comment:
Shouldn't that than be a empty string ""?
```suggestion
return "";
```
(Dislaimer: not a C expert ;)
##########
File path: build-utils/language-c/src/main/resources/templates/c/enum-template-h.ftlh
##########
@@ -50,6 +50,7 @@ ${helper.getIncludesDirectory()?replace(".", "/")}/${helper.camelCaseToSnakeCase
</#list>
</#if>
+
Review comment:
accidental newline ?
##########
File path: build-utils/language-go/src/main/resources/templates/go/data-io-template.ftlh
##########
@@ -75,7 +75,8 @@ import (
func ${type.name}Parse(io *utils.ReadBuffer<#if type.parserArguments?has_content>, <#list type.parserArguments as parserArgument>${parserArgument.name} <#if helper.isComplexTypeReference(parserArgument.type) && !helper.isEnumTypeReference(parserArgument.type)>I</#if>${helper.getLanguageTypeNameForTypeReference(parserArgument.type)}<#sep>, </#sep></#list></#if>) (api.PlcValue, error) {
switch {
<#list type.switchField.cases as case>
- <#if case.discriminatorValues?has_content>case <#list case.discriminatorValues as discriminatorValue>${helper.toParseExpression(null, type.switchField.discriminatorExpressions[discriminatorValue?index], type.parserArguments)} == <#if helper.discriminatorValueNeedsStringEqualityCheck(type.switchField.discriminatorExpressions[discriminatorValue?index])>"${discriminatorValue}"<#elseif helper.isEnumExpression(discriminatorValue)>${helper.getEnumExpression(discriminatorValue)}<#else>${discriminatorValue}</#if><#sep> && </#sep></#list><#else>default</#if>: // ${case.name}
+ <#if case.discriminatorValues?has_content>case <#list case.discriminatorValues as discriminatorValue>${helper.toParseExpression(null, type.switchField.discriminatorExpressions[discriminatorValue?index], type.parserArguments)} == <#if helper.discriminatorValueNeedsStringEqualityCheck(type.switchField.discriminatorExpressions[discriminatorValue?index])>"${discriminatorValue}"<#elseif helper.isComplexTypeReference(type.parserArguments[discriminatorValue?index].type)><#if helper.isEnumTypeReference(type.parserArguments[discriminatorValue?index].type)>${helper.getLanguageTypeNameForTypeReference(type.parserArguments[discriminatorValue?index].type)}_${discriminatorValue}<#else>${discriminatorValue}</#if><#else>${discriminatorValue}</#if><#sep> && </#list> <#else>default</#if>: // ${case.name}
+
Review comment:
accidental newline?
```suggestion
```
##########
File path: build-utils/language-go/src/main/resources/templates/go/model-template.ftlh
##########
@@ -306,10 +322,18 @@ func (m *${type.name}) LengthInBits() uint16 {
<#case "simple">
<#assign simpleField = field>
+
Review comment:
Accidental newline(s)?
```suggestion
```
##########
File path: build-utils/language-java/pom.xml
##########
@@ -55,4 +55,39 @@
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-invoker-plugin</artifactId>
+ <version>3.1.0</version>
+ <configuration>
+ <debug>true</debug>
+ <projectsDirectory>src/test/resources</projectsDirectory>
+ <cloneProjectsTo>${project.build.directory}/integration-tests</cloneProjectsTo>
+ <!-- Removed so that a local version of the build-tools could be used. Need to add it back in -->
+ <!-- localRepositoryPath>${project.build.directory}/local-repo</localRepositoryPath -->
+ <settingsFile>src/test/resources/settings.xml</settingsFile>
+ <pomIncludes>
+ <pomInclude>*/pom.xml</pomInclude>
+ </pomIncludes>
+ <goals>
+ <goal>test</goal>
+ </goals>
+ </configuration>
+ <executions>
+ <execution>
+ <id>integration-test</id>
+ <goals>
+ <goal>install</goal>
+ <goal>integration-test</goal>
+ <goal>verify</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+
+
</project>
Review comment:
new line at EOF
```suggestion
</project>
```
##########
File path: build-utils/protocol-base-mspec/src/main/java/org/apache/plc4x/plugins/codegenerator/language/mspec/parser/MessageFormatListener.java
##########
@@ -42,6 +42,8 @@ Licensed to the Apache Software Foundation (ASF) under one
public class MessageFormatListener extends MSpecBaseListener {
+
Review comment:
```suggestion
```
accidental newlines?
##########
File path: build-utils/protocol-base-mspec/src/main/java/org/apache/plc4x/plugins/codegenerator/language/mspec/model/definitions/DefaultDataIoTypeDefinition.java
##########
@@ -21,18 +21,24 @@ Licensed to the Apache Software Foundation (ASF) under one
import org.apache.plc4x.plugins.codegenerator.types.definitions.Argument;
import org.apache.plc4x.plugins.codegenerator.types.definitions.DataIoTypeDefinition;
import org.apache.plc4x.plugins.codegenerator.types.fields.SwitchField;
+import org.apache.plc4x.plugins.codegenerator.types.references.DefaultComplexTypeReference;
+import org.apache.plc4x.plugins.codegenerator.types.references.TypeReference;
public class DefaultDataIoTypeDefinition extends DefaultTypeDefinition implements DataIoTypeDefinition {
private final SwitchField switchField;
+ private final TypeReference type;
public DefaultDataIoTypeDefinition(String name, Argument[] parserArguments, String[] tags, SwitchField switchField) {
super(name, parserArguments, tags);
this.switchField = switchField;
+ this.type = parserArguments[0].getType();
}
public SwitchField getSwitchField() {
return switchField;
}
+ public TypeReference getType() { return this.type; }
Review comment:
```suggestion
public TypeReference getType() {
return this.type;
}
```
##########
File path: build-utils/protocol-base-mspec/src/test/java/org/apache/plc4x/plugins/codegenerator/language/mspec/parser/MessageFormatParserTest.java
##########
@@ -39,5 +41,6 @@ void parseNull() {
void parseSomething() {
Map<String, TypeDefinition> parse = SUT.parse(getClass().getResourceAsStream("/mspec.example"));
System.out.println(parse);
+
}
}
Review comment:
newline at EOF
```suggestion
}
```
##########
File path: build-utils/language-c/src/main/java/org/apache/plc4x/language/c/CLanguageTemplateHelper.java
##########
@@ -178,33 +178,44 @@ public boolean requiresPointerAccess(ComplexTypeDefinition typeDefinition, Strin
*/
@Override
public String getLanguageTypeNameForTypeReference(TypeReference typeReference) {
+ System.out.println("Type Reference " + typeReference.getClass());
Review comment:
```suggestion
```
Debug statement I guess (We might add more logging to the whole language helper in the future I reckon).
##########
File path: protocols/ads/src/main/resources/protocols/ads/ads.mspec
##########
@@ -386,7 +386,7 @@
[array int 8 'data' count 'sampleSize']
]
-[dataIo 'DataItem' [string 'dataFormatName', int 32 'stringLength']
+[dataIo 'DataItem' [string '-1' 'dataFormatName', int 32 'stringLength']
Review comment:
what does '-1' mean for a string?
##########
File path: build-utils/language-c/pom.xml
##########
@@ -55,4 +89,5 @@
</dependency>
</dependencies>
+
Review comment:
Accidental newline?
```suggestion
```
##########
File path: build-utils/language-c/src/main/java/org/apache/plc4x/language/c/CLanguageTemplateHelper.java
##########
@@ -178,33 +178,44 @@ public boolean requiresPointerAccess(ComplexTypeDefinition typeDefinition, Strin
*/
@Override
public String getLanguageTypeNameForTypeReference(TypeReference typeReference) {
+ System.out.println("Type Reference " + typeReference.getClass());
if (typeReference instanceof SimpleTypeReference) {
SimpleTypeReference simpleTypeReference = (SimpleTypeReference) typeReference;
switch (simpleTypeReference.getBaseType()) {
case BIT:
return "bool";
- case UINT:
+
Review comment:
```suggestion
```
accidental newline?
##########
File path: build-utils/language-c/src/main/resources/templates/c/data-io-template-c.ftlh
##########
@@ -67,7 +67,7 @@
#include <plc4c/data.h>
#include <plc4c/utils/list.h>
#include <plc4c/spi/evaluation_helper.h>
-#include <plc4c/driver_${helper.getProtocolName()}.h>
+
Review comment:
could be accidental newline but I guess this is the C-like import grouping? Would than expect one after time.h too
##########
File path: build-utils/language-c/src/main/resources/templates/c/pojo-template-c.ftlh
##########
@@ -221,7 +227,7 @@ plc4c_return_code ${helper.getCTypeName(type.name)}_parse(plc4c_spi_read_buffer*
<#if indentContent> </#if> // Checksum Field (${checksumField.name})
<#if indentContent> </#if> {
<#if indentContent> </#if> // Create an array of all the bytes read in this message element so far.
-<#if indentContent> </#if> byte[] checksumRawData = plc4c_spi_read_get_bytes(io, startPos, plc4c_spi_read_get_pos(io));
+<#if indentContent> </#if> //unsigned char checksumRawData[] = plc4c_spi_read_get_bytes(io, startPos, plc4c_spi_read_get_pos(io));
Review comment:
is that meant to be outcommeted?
```suggestion
<#if indentContent> </#if> unsigned char checksumRawData[] = plc4c_spi_read_get_bytes(io, startPos, plc4c_spi_read_get_pos(io));
```
##########
File path: build-utils/language-go/src/main/java/org/apache/plc4x/language/go/GoLanguageTemplateHelper.java
##########
@@ -309,7 +309,7 @@ public String getReadBufferReadMethodCall(SimpleTypeReference simpleTypeReferenc
}
case STRING: {
StringTypeReference stringTypeReference = (StringTypeReference) simpleTypeReference;
- return "io.ReadString(" + stringTypeReference.getSizeInBits() + ")";
+ return "io.ReadString(" + stringTypeReference.getLength() + ")";
Review comment:
see other bit size vs byte site length comment
##########
File path: build-utils/language-c/src/main/java/org/apache/plc4x/language/c/CLanguageTemplateHelper.java
##########
@@ -380,7 +390,7 @@ public String getReadBufferReadMethodCall(SimpleTypeReference simpleTypeReferenc
throw new FreemarkerException("Unsupported float type with " + floatTypeReference.getSizeInBits() + " bits");
case STRING:
StringTypeReference stringTypeReference = (StringTypeReference) simpleTypeReference;
- return "plc4c_spi_read_string(io, " + stringTypeReference.getSizeInBits() + ", \"" +
+ return "plc4c_spi_read_string(io, " + stringTypeReference.getLength() + " * 8, \"" +
Review comment:
What is the reason using bytes sized lengths here? Isn't that supported for strings?
##########
File path: build-utils/language-go/src/main/java/org/apache/plc4x/language/go/GoLanguageTemplateHelper.java
##########
@@ -368,7 +368,7 @@ public String getWriteBufferWriteMethodCall(SimpleTypeReference simpleTypeRefere
StringTypeReference stringTypeReference = (StringTypeReference) simpleTypeReference;
String encoding = ((stringTypeReference.getEncoding() != null) && (stringTypeReference.getEncoding().length() > 2)) ?
stringTypeReference.getEncoding().substring(1, stringTypeReference.getEncoding().length() - 1) : "UTF-8";
- return "io.WriteString(" + stringTypeReference.getSizeInBits() + ", \"" +
+ return "io.WriteString(" + stringTypeReference.getLength() + ", \"" +
Review comment:
see other bit size vs byte site length comment
##########
File path: build-utils/language-c/src/test/resources/settings.xml
##########
@@ -0,0 +1,51 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ -->
+<settings>
+ <profiles>
+ <profile>
+ <id>it-repo</id>
+ <activation>
+ <activeByDefault>true</activeByDefault>
+ </activation>
+ <repositories>
+ <repository>
+ <id>local.central</id>
+ <url>@localRepositoryUrl@</url>
+ <releases>
+ <enabled>true</enabled>
+ </releases>
+ <snapshots>
+ <enabled>true</enabled>
+ </snapshots>
+ </repository>
+ </repositories>
+ <pluginRepositories>
+ <pluginRepository>
+ <id>local.central</id>
+ <url>@localRepositoryUrl@</url>
+ <releases>
+ <enabled>true</enabled>
+ </releases>
+ <snapshots>
+ <enabled>true</enabled>
+ </snapshots>
+ </pluginRepository>
+ </pluginRepositories>
+ </profile>
+ </profiles>
+</settings>
Review comment:
EOL at end of file
```suggestion
</settings>
```
##########
File path: build-utils/language-go/src/main/resources/templates/go/model-template.ftlh
##########
@@ -360,6 +384,7 @@ func ${type.name}Parse(io *utils.ReadBuffer<#if type.parserArguments?has_content
<#if (!helper.isSimpleTypeReference(arrayField.type)) && helper.requiresVariable(arrayField, "lastItem")>
lastItem := curItem == uint16(${helper.toParseExpression(arrayField, arrayField.loopExpression, type.parserArguments)} - 1)
</#if>
+
Review comment:
accidental newline?
```suggestion
```
##########
File path: build-utils/language-go/src/main/resources/templates/go/model-template.ftlh
##########
@@ -306,10 +322,18 @@ func (m *${type.name}) LengthInBits() uint16 {
<#case "simple">
<#assign simpleField = field>
+
+
Review comment:
Accidental newline(s)?
```suggestion
```
##########
File path: build-utils/language-java/src/main/java/org/apache/plc4x/language/java/JavaLanguageTemplateHelper.java
##########
@@ -363,7 +367,7 @@ public String getReadBufferReadMethodCall(SimpleTypeReference simpleTypeReferenc
}
case STRING: {
StringTypeReference stringTypeReference = (StringTypeReference) simpleTypeReference;
- return "io.readString(" + stringTypeReference.getSizeInBits() + ", \"" +
+ return "io.readString(" + stringTypeReference.getLength() + ", \"" +
Review comment:
see other bit length vs byte length comment
##########
File path: build-utils/language-go/src/test/resources/plc4go/pom.xml
##########
@@ -0,0 +1,323 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review comment:
was just wondering if we find a way to keep these in sync with the other poms or if we use tool native builds here.
##########
File path: build-utils/language-java/src/main/resources/templates/java/io-template.ftlh
##########
@@ -246,7 +246,12 @@ public class ${type.name}IO implements <#if outputFlavor != "passive">MessageIO<
<#assign simpleTypeReference = discriminatorField.type>
// Discriminator Field (${discriminatorField.name}) (Used as input to a switch field)
- ${helper.getLanguageTypeNameForField(field)} ${discriminatorField.name} = ${helper.getReadBufferReadMethodCall(simpleTypeReference)};
+ <#if helper.isEnumField(field)>
+ ${helper.getLanguageTypeNameForField(field)} ${discriminatorField.name} = ${helper.getLanguageTypeNameForField(discriminatorField)}.enumForValue(${helper.getReadBufferReadMethodCall(helper.getEnumBaseTypeReference(discriminatorField.type))});
+ <#else>
+ ${helper.getLanguageTypeNameForField(field)} ${discriminatorField.name} = <#if helper.isSimpleTypeReference(discriminatorField.type)>${helper.getReadBufferReadMethodCall(discriminatorField.type)}<#else>${discriminatorField.type.name}IO.staticParse(io<#if field.params?has_content>, <#list field.params as parserArgument>(${helper.getLanguageTypeNameForTypeReference(helper.getArgumentType(discriminatorField.type, parserArgument?index), true)}) (${helper.toParseExpression(discriminatorField, parserArgument, type.parserArguments)})<#sep>, </#sep></#list></#if>)</#if>;
+ </#if>
+
Review comment:
```suggestion
```
accidental newline?
##########
File path: build-utils/language-go/src/main/resources/templates/go/model-template.ftlh
##########
@@ -306,10 +322,18 @@ func (m *${type.name}) LengthInBits() uint16 {
<#case "simple">
<#assign simpleField = field>
+
+
+
Review comment:
Accidental newline(s)?
```suggestion
```
##########
File path: build-utils/language-java/src/main/resources/templates/java/enum-template.ftlh
##########
@@ -59,7 +59,7 @@ import java.util.Map;
public enum ${type.name} {
<#list type.enumValues as enumValue>
- ${enumValue.name}(<#if type.type?has_content>(${helper.getLanguageTypeNameForTypeReference(type.type, true)}) <#if helper.isStringTypeReference(type.type)>"${enumValue.value}"<#else>${enumValue.value}</#if></#if><#if type.constantNames?has_content><#if type.type?has_content>, </#if><#list type.constantNames as constantName>(${helper.getLanguageTypeNameForTypeReference(type.getConstantType(constantName), true)}) ${helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName))}<#sep>, </#sep></#list></#if>)<#sep>,
+ ${enumValue.name}(<#if type.type?has_content>(${helper.getLanguageTypeNameForTypeReference(type.type, true)}) <#if helper.isStringTypeReference(type.type)>"${enumValue.value}"<#elseif helper.isComplexTypeReference(type.type)><#if helper.isEnumTypeReference(type.type)>${helper.getLanguageTypeNameForTypeReference(type.type, true)}.${enumValue.value}<#else>${enumValue.value}</#if><#else>${enumValue.value}</#if></#if><#if type.constantNames?has_content><#if type.type?has_content>, </#if><#list type.constantNames as constantName><#if helper.isComplexTypeReference(type.getConstantType(constantName))><#if helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName)) == 'null'>null<#elseif helper.isEnumTypeReference(type.getConstantType(constantName))>${helper.getLanguageTypeNameForTypeReference(type.getConstantType(constantName), true)}.${helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName))}<#else>(${helper.getLanguag
eTypeNameForTypeReference(type.getConstantType(constantName), true)}) ${helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName))}</#if><#else>(${helper.getLanguageTypeNameForTypeReference(type.getConstantType(constantName), true)}) ${helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName))}</#if><#sep>, </#sep></#list></#if>)<#sep>,
Review comment:
general comment: we should research if we can add some line breaks here 😄
##########
File path: build-utils/language-java/src/main/resources/templates/java/data-io-template.ftlh
##########
@@ -311,7 +313,9 @@ public class ${type.name}IO {
public static WriteBuffer staticSerialize(PlcValue _value<#if type.parserArguments?has_content>, <#list type.parserArguments as parserArgument>${helper.getLanguageTypeNameForTypeReference(parserArgument.type, false)} ${parserArgument.name}<#sep>, </#sep></#list></#if>, boolean littleEndian) throws ParseException {
<#assign defaultCaseOutput=false>
- <#list type.switchField.cases as case><#if case.discriminatorValues?has_content>if(<#list case.discriminatorValues as discriminatorValue>EvaluationHelper.equals(${helper.toParseExpression(null, type.switchField.discriminatorExpressions[discriminatorValue?index], type.parserArguments)}, <#if helper.discriminatorValueNeedsStringEqualityCheck(type.switchField.discriminatorExpressions[discriminatorValue?index])>"${discriminatorValue}"<#else>${discriminatorValue}</#if>)<#sep> && </#sep></#list>) <#else><#assign defaultCaseOutput=true></#if>{ // ${case.name}
+ <#list type.switchField.cases as case>
+ <#if case.discriminatorValues?has_content>if(<#list case.discriminatorValues as discriminatorValue>EvaluationHelper.equals(${helper.toParseExpression(null, type.switchField.discriminatorExpressions[discriminatorValue?index], type.parserArguments)},<#if helper.discriminatorValueNeedsStringEqualityCheck(type.switchField.discriminatorExpressions[discriminatorValue?index])>"${discriminatorValue}"<#elseif helper.isComplexTypeReference(type.parserArguments[discriminatorValue?index].type)><#if helper.isEnumTypeReference(type.parserArguments[discriminatorValue?index].type)>${helper.getLanguageTypeNameForTypeReference(type.parserArguments[discriminatorValue?index].type, false)}.${discriminatorValue}<#else>${discriminatorValue}</#if><#else>${discriminatorValue}</#if>)<#sep> && </#sep></#list>) <#else><#assign defaultCaseOutput=true></#if>{ // ${case.name}
+
Review comment:
```suggestion
```
accidental newline?
##########
File path: build-utils/language-java/src/main/java/org/apache/plc4x/language/java/JavaLanguageTemplateHelper.java
##########
@@ -422,7 +425,7 @@ public String getWriteBufferWriteMethodCall(SimpleTypeReference simpleTypeRefere
}
case STRING: {
StringTypeReference stringTypeReference = (StringTypeReference) simpleTypeReference;
- return "io.writeString(" + stringTypeReference.getSizeInBits() + ", \"" +
+ return "io.writeString(" + stringTypeReference.getLength() + ", \"" +
Review comment:
see other bit length vs byte length comment
##########
File path: build-utils/protocol-base-mspec/src/main/java/org/apache/plc4x/plugins/codegenerator/language/mspec/parser/MessageFormatListener.java
##########
@@ -42,6 +42,8 @@ Licensed to the Apache Software Foundation (ASF) under one
public class MessageFormatListener extends MSpecBaseListener {
+
+
Review comment:
```suggestion
```
accidental newlines?
##########
File path: build-utils/language-java/src/test/resources/integration-test/pom.xml
##########
@@ -0,0 +1,224 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review comment:
same as other comment about test-pom.xml: is there a way to keep them easy in sync
##########
File path: build-utils/protocol-base-mspec/src/test/java/org/apache/plc4x/plugins/codegenerator/language/mspec/parser/MessageFormatParserTest.java
##########
@@ -39,5 +41,6 @@ void parseNull() {
void parseSomething() {
Map<String, TypeDefinition> parse = SUT.parse(getClass().getResourceAsStream("/mspec.example"));
System.out.println(parse);
+
Review comment:
accidental newline?
```suggestion
```
##########
File path: build-utils/protocol-test/pom.xml
##########
@@ -74,13 +215,6 @@
<version>0.9.0-SNAPSHOT</version>
</dependency>
- <dependency>
- <groupId>org.apache.plc4x</groupId>
- <artifactId>plc4x-build-utils-language-java</artifactId>
- <version>0.9.0-SNAPSHOT</version>
- <!-- Scope is 'provided' as this way it's not shipped with the driver -->
- <scope>provided</scope>
- </dependency>
</dependencies>
</project>
Review comment:
newline at EOF
```suggestion
</project>
```
##########
File path: build-utils/protocol-test/src/main/resources/protocols/test/test.mspec
##########
@@ -21,50 +21,319 @@
// Simple Type
////////////////////////////////////////////////////////////////
-[type 'SimpleType'
- [array uint 8 'arrCount' count '5']
- [array uint 8 'arrLen' length '6']
- [array uint 8 'arrTerm' terminated 'terminationExpression']
- [checksum uint 8 'crc' 'checksumExpression']
- [const uint 8 'con' '0x03']
- [implicit uint 8 'impl' 'serializeExpression']
- [manualArray uint 8 'manArrayCount' count 'loopExpression' 'serializationExpression' 'deserializationExpression' 'lengthExpression']
- [manualArray uint 8 'manArrayLen' length 'loopExpression' 'serializationExpression' 'deserializationExpression' 'lengthExpression']
- [manualArray uint 8 'manArrayTerm' terminated 'loopExpression' 'serializationExpression' 'deserializationExpression' 'lengthExpression']
- [manual uint 8 'man' 'serializationExpression' 'deserializationExpression' 'lengthExpression']
- [optional uint 8 'opt' 'optionalExpression']
- [padding uint 8 'pad' '0' 'paddingExpression']
- [reserved uint 8 '0x00']
- [simple uint 8 'simp']
- [virtual uint 8 'virt' 'valueExpression']
+[type 'FieldTypeTest'
+ [simple uint 8 'simpleField']
+ [abstract unit 8 'abstractField']
+ [array uint 8 'arrayField' count '5']
+ //Checksums fields are not supported in C
+ //[checksum uint 8 'checksumField' '100']
+ [const uint 8 'constField' '5']
+
+ //Discriminated Field can't be used in simple type
+ //[discriminator uint 8 'discriminatorField']
+
+ [enum EnumType 'enumField']
+ [implicit uint 8 'implicitField' 'simpleField']
+ [optional uint 8 'optionalField' 'simpleField == 5']
+ [padding uint 8 'paddingField' '0x00' 'simpleField']
+ [reserved uint 8 '0x00']
+
+ //TypeSwitch field can't be used in non disriminatedTypes
+ //[typeSwitch 'simpleField' ]
+]
+
+//The following data types are not yet implemented but have a reference
+//Not Implemented Yet In
+//-Java
+//[simple ufloat 8.23 'ufloatField']
+
+//Not Implemented Yet In
+//-Java
+//[simple ufloat 11.52 'udoubleField']
+
+//Not Implemented Yet In
+//-Java
+//[simple time 8 'timeField']
+
+//Not Implemented Yet In
+//-Java
+//[simple date 8 'dateField']
+
+//Not Implemented Yet In
+//-Java
+//[simple dateTime 8 'dateTimeField']
+
+[type 'SimpleTypeTest'
+ [simple bit 'bitField']
+ [simple int 8 'intField']
+ [simple uint 8 'uintField']
+ [simple float 8.23 'floatField']
+ [simple float 11.52 'doubleField']
+ [simple string '8' 'UTF-8' 'stringField']
+]
+
+
+//Abstract fields don't require the 'errors' module in Go, this causes an unused import error.
+//[type 'AbstractTypeTest'
+// [abstract bit 'bitField']
+// [abstract int 8 'intField']
+// [abstract uint 8 'uintField']
+// [abstract float 8.23 'floatField']
+// [abstract float 11.52 'doubleField']
+// [abstract string '8' 'UTF-8' 'stringField']
+//]
+
+[type 'ArrayTypeTest'
+ [array bit 'bitField' count '5']
+ [array int 8 'intField' count '5']
+ [array uint 8 'uintField' count '5']
+ [array float 8.23 'floatField' count '5']
+ [array float 11.52 'doubleField' count '5']
+ [array string '8' 'UTF-8' 'stringField' count '5']
+]
+
+//Checksums fields are not supported in C
+//[type 'CheckSumTypeTest'
+ //Bit field cannot be used for a checksum
+ //[checksum bit 'bitField' true]
+ //[checksum int 8 'intField' '100']
+ //[checksum uint 8 'uintField' '100']
+ //Float fields cannot be used as checksums
+ //[checksum float 8.23 'floatField' '100.0f']
+ //[checksum float 11.52 'doubleField' '100.0']
+ //String field cannot be used as a checksum
+ //[checksum string '11 * 8' 'UTF-8' 'stringField' '"HELLO TODDY"']
+//]
+
+[type 'ConstTypeTest'
+ [const bit 'bitField' true]
+ [const int 8 'intField' '100']
+ [const uint 8 'uintField' '100']
+ [const float 8.23 'floatField' '100.0']
+ [const float 11.52 'doubleField' '100.0']
+ [const string '8' 'UTF-8' 'stringField' '"HELLO TODDY"']
+]
+
+[type 'EnumTypeTest'
+ [enum EnumType 'enumField']
+]
+
+[type 'ImplicitTypeTest'
+ //Implicit types have the requirement that the expression is of a similar type to the field
+ //i.e Integers can't be cast to Booleans
+ [simple uint 8 'simpleField']
+
+ [implicit bit 'bitField' 'simpleField > 0']
+ [implicit int 8 'intField' 'simpleField']
+ [implicit uint 8 'uintField' 'simpleField']
+ [implicit float 8.23 'floatField' 'simpleField']
+ [implicit float 11.52 'doubleField' 'simpleField']
+ //String literals can't be used in the expression
+ //[implicit string '8' 'UTF-8' 'stringField' 'simpleField > 0 ? "HELLO TODDY" : "BYE TODDY"']
+]
+
+[type 'OptionalTypeTest'
+ [simple uint 8 'simpleField']
+ [optional uint 8 'optionalField' 'simpleField == 5']
+]
+
+[type 'PaddingTypeTest'
+ [simple uint 8 'simpleField']
+ [padding uint 8 'paddingField' '0x00' 'simpleField']
+]
+
+[type 'ReservedTypeTest'
+ [reserved uint 8 '0x00']
+]
+
+[type 'IntTypeTest'
+ [simple int 3 'ThreeField']
+ [simple int 8 'ByteField']
+ [simple int 16 'WordField']
+ [simple int 24 'WordPlusByteField']
+ [simple int 32 'DoubleIntField']
+ [simple int 64 'QuadIntField']
+]
+
+[type 'UIntTypeTest'
+ [simple uint 3 'ThreeField']
+ [simple uint 8 'ByteField']
+ [simple uint 16 'WordField']
+ [simple uint 24 'WordPlusByteField']
+ [simple uint 32 'DoubleIntField']
+ [simple uint 64 'QuadIntField']
]
////////////////////////////////////////////////////////////////
-// Discriminated Type
+// Discriminated Type Tests
////////////////////////////////////////////////////////////////
-[discriminatedType 'DiscriminatedType'
+[discriminatedType 'EnumDiscriminatedType'
+ [discriminator EnumType 'discr']
+ [typeSwitch 'discr'
+ ['BOOL' EnumDiscriminatedTypeA
+ [simple uint 8 'simpA']
+ ]
+ ['UINT' EnumDiscriminatedTypeB
+ [simple uint 8 'simpB']
+ ]
+ ['INT' EnumDiscriminatedTypeC
+ [simple uint 8 'simpC']
+ ]
+ ]
+]
+
+// Multiple Enumerated discriminators
+[discriminatedType 'EnumDiscriminatedTypeMultiple'
+ [discriminator EnumType 'discr1']
+ [discriminator EnumTypeInt 'discr2']
+ [typeSwitch 'discr1','discr2'
+ ['BOOL','BOOLINT' EnumDiscriminatedTypeMultipleA
+ [simple uint 8 'simpA']
+ ]
+ ['UINT','UINTINT' EnumDiscriminatedTypeMultipleB
+ [simple uint 8 'simpB']
+ ]
+ ['INT','INTINT' EnumDiscriminatedTypeMultipleC
+ [simple uint 8 'simpC']
+ ]
+ ]
+]
+
+// Enumerated Parameter
+[discriminatedType 'EnumDiscriminatedTypeParameter' [EnumType 'discr']
+ [typeSwitch 'discr'
+ ['BOOL' EnumDiscriminatedTypeAParameter
+ [simple uint 8 'simpA']
+ ]
+ ['UINT' EnumDiscriminatedTypeBParameter
+ [simple uint 8 'simpB']
+ ]
+ ['INT' EnumDiscriminatedTypeCParameter
+ [simple uint 8 'simpC']
+ ]
+ ]
+]
+
+// Multiple Enumerated Parameters
+[discriminatedType 'EnumDiscriminatedTypeParameterMultiple' [EnumType 'discr1', EnumTypeInt 'discr2']
+ [typeSwitch 'discr1','discr2'
+ ['BOOL','BOOLINT' EnumDiscriminatedTypeAParameterMultiple
+ [simple uint 8 'simpA']
+ ]
+ ['UINT','UINTINT' EnumDiscriminatedTypeBParameterMultiple
+ [simple uint 8 'simpB']
+ ]
+ ['INT','INTINT' EnumDiscriminatedTypeCParameterMultiple
+ [simple uint 8 'simpC']
+ ]
+ ]
+]
+
+[discriminatedType 'SimpleDiscriminatedType'
[discriminator uint 8 'discr']
[typeSwitch 'discr'
- ['0x01' DiscriminatedTypeA
+ ['0x00' SimpleDiscriminatedTypeA
[simple uint 8 'simpA']
]
- ['0x02' DiscriminatedTypeA
+ ['0x01' SimpleDiscriminatedTypeB
[simple uint 8 'simpB']
]
- ['0x03' DiscriminatedTypeA
+ ['0x02' SimpleDiscriminatedTypeC
[simple uint 8 'simpC']
]
]
]
////////////////////////////////////////////////////////////////
-// Arguments Type
+// Enumerated Type Tests
////////////////////////////////////////////////////////////////
+//Not really useful, but this uses the pojo templates instead of the enum templates
+[enum int 8 'EnumEmpty'
+
+]
+
+// Go doesn't support Enumerated Bits
+//[enum bit 'EnumTypeBit'
+// ['true' TRUE]
+// ['false' FALSE]
+//]
+
+[enum int 8 'EnumTypeInt'
+ ['0x01' BOOLINT]
+ ['0x02' UINTINT]
+ ['0x03' INTINT]
+]
+
+[enum uint 8 'EnumType'
+ ['0x01' BOOL]
+ ['0x02' UINT]
+ ['0x03' INT]
+]
+
+// C doesn't support non integer switch fields
+//[enum float 8.23 'EnumTypeFloat'
+// ['100.0' LOW]
+// ['101.0' MID]
+// ['102.0' BIG]
+//]
+
+// C doesn't support non integer switch fields
+//[enum float 11.52 'EnumTypeDouble'
+// ['100.0' LOW]
+// ['101.0' MID]
+// ['102.0' BIG]
+//]
+
+//String based enum's needs some work in C, compiles but assigns 0 as values.
+[enum string '-1' 'EnumTypeString'
+ ['Toddy1' TODDY]
+]
+
+// Fails to import the base Enum in C, need to find it in getComplexTypeReferences
+//[enum EnumType 'EnumTypeEnum'
+// ['BOOL' BOOL]
+// ['UINT' UINT]
+// ['INT' INT]
+//]
+
+// Float parameters aren't implemented for constants in enums in C
+//[enum int 8 'EnumTypeAllTest' [bit 'bitType', int 8 'intType', uint 8 'uintType', float 8.23 'floatType', float 11.52 'doubleType', string '-1' 'stringType', EnumType 'enumType']
+// ['0x01' BOOL ['false' , '1' , '1' , '100.0' , '100.0' , 'IEC61131_BOOL' , 'BOOL']]
+// ['0x02' BYTE ['true' , '2' , '2' , '101.1' , '101.1' , 'IEC61131_BYTE' , 'UINT']]
+//]
+
+// Keyword named parameters aren't allowed
+//[enum int 8 'EnumTypeIntTest' [int 8 'int']
+// ['0x01' BOOL ['1']]
+// ['0x02' BYTE ['2']]
+//]
+
+//Showing allowed parameter types for enums
+[enum int 8 'EnumTypeParameters' [bit 'bitType', int 8 'intType', uint 8 'uintType', string '-1' 'stringType', EnumType 'enumType']
+ ['0x01' BOOL ['false' , '1' , '1' , 'IEC61131_BOOL' , 'BOOL']]
+ ['0x02' BYTE ['true' , '2' , '2' , 'IEC61131_BYTE' , 'UINT']]
+]
////////////////////////////////////////////////////////////////
-// Discriminated Type with multiple conditions
+// Data IO Tests
////////////////////////////////////////////////////////////////
+[dataIo 'DataIOTypeEmpty'
+]
+
+[dataIo 'DataIOType' [EnumType 'dataType']
+ [typeSwitch 'dataType'
+ ['BOOL' BOOL
+ [simple bit 'value']
+ ]
+ ['UINT' USINT
+ [simple uint 8 'value']
+ ]
+ ['INT' UINT
+ [simple uint 16 'value']
+ ]
+ ]
+]
Review comment:
newline at EOF
```suggestion
]
```
##########
File path: go.mod
##########
@@ -19,3 +19,5 @@
module github.com/apache/plc4x
go 1.15
+
Review comment:
This toplevel go.mod file confuses me. What is the purpose of that? I open plc4go as TLP in golang so maybe this is from opening the whole plc4x in goland?
##########
File path: build-utils/protocol-base-mspec/src/test/resources/mspec.example
##########
@@ -355,4 +355,11 @@
[simple uint 32 'sampleSize']
// n Bytes Data
[array int 8 'data' count 'sampleSize']
+]
+
+//Specific Case for variable string length
+[type 'VariableStringType'
+ //Confirm implicit can be used in string length
+ [implicit uint 32 'stringLength' 'stringValue.lengthInBytes']
+ [simple string 'stringLength' 'stringValue']
]
Review comment:
newline at EOF
```suggestion
]
```
##########
File path: plc4go/internal/plc4go/ads/readwrite/model/AdsData.go
##########
@@ -98,45 +98,65 @@ func AdsDataParse(io *utils.ReadBuffer, commandId *CommandId, response bool) (*A
var _parent *AdsData
var typeSwitchError error
switch {
- case *commandId == CommandId_INVALID && response == false:
+
Review comment:
Guess all these are artifacts from the superfluous newlines...
##########
File path: build-utils/protocol-base-mspec/src/test/resources/mspec.example
##########
@@ -208,9 +208,9 @@
[discriminatedType 'ADSData' [CommandId 'commandId', bit 'response']
[typeSwitch 'commandId', 'response'
- ['CommandId.INVALID', 'true' AdsInvalidResponse]
- ['CommandId.INVALID', 'false' AdsInvalidRequest]
- ['CommandId.ADS_READ_DEVICE_INFO', 'true' AdsReadDeviceInfoResponse
+ ['INVALID', 'true' AdsInvalidResponse]
Review comment:
Think @chrisdutz talked about that: This is still a reference to a existing enum not a string right? Like you don't need to qualify them here
##########
File path: build-utils/protocol-test/src/main/resources/protocols/test/test.mspec
##########
@@ -21,50 +21,319 @@
// Simple Type
Review comment:
Maybe add a comment about the purpose of this file here like:
```suggestion
////////////////////////////////////////////////////////////////
// This file is a crossproduct of all mspec features
////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////
// Simple Type
```
##########
File path: build-utils/language-java/src/main/resources/templates/java/enum-template.ftlh
##########
@@ -59,7 +59,7 @@ import java.util.Map;
public enum ${type.name} {
<#list type.enumValues as enumValue>
- ${enumValue.name}(<#if type.type?has_content>(${helper.getLanguageTypeNameForTypeReference(type.type, true)}) <#if helper.isStringTypeReference(type.type)>"${enumValue.value}"<#else>${enumValue.value}</#if></#if><#if type.constantNames?has_content><#if type.type?has_content>, </#if><#list type.constantNames as constantName>(${helper.getLanguageTypeNameForTypeReference(type.getConstantType(constantName), true)}) ${helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName))}<#sep>, </#sep></#list></#if>)<#sep>,
+ ${enumValue.name}(<#if type.type?has_content>(${helper.getLanguageTypeNameForTypeReference(type.type, true)}) <#if helper.isStringTypeReference(type.type)>"${enumValue.value}"<#elseif helper.isComplexTypeReference(type.type)><#if helper.isEnumTypeReference(type.type)>${helper.getLanguageTypeNameForTypeReference(type.type, true)}.${enumValue.value}<#else>${enumValue.value}</#if><#else>${enumValue.value}</#if></#if><#if type.constantNames?has_content><#if type.type?has_content>, </#if><#list type.constantNames as constantName><#if helper.isComplexTypeReference(type.getConstantType(constantName))><#if helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName)) == 'null'>null<#elseif helper.isEnumTypeReference(type.getConstantType(constantName))>${helper.getLanguageTypeNameForTypeReference(type.getConstantType(constantName), true)}.${helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName))}<#else>(${helper.getLanguag
eTypeNameForTypeReference(type.getConstantType(constantName), true)}) ${helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName))}</#if><#else>(${helper.getLanguageTypeNameForTypeReference(type.getConstantType(constantName), true)}) ${helper.escapeValue(type.getConstantType(constantName), enumValue.getConstant(constantName))}</#if><#sep>, </#sep></#list></#if>)<#sep>,
Review comment:
in all other places where we get such long lines too 😉
##########
File path: go.sum
##########
@@ -0,0 +1,16 @@
+github.com/ajankovic/xdiff v0.0.1/go.mod h1:SUmEZ67uB97I0zkiuQ+lb+LOms9ipn8X+p+2RdJV710=
Review comment:
see go.mod comment above
##########
File path: pom.xml
##########
@@ -100,7 +100,7 @@
<!-- Exclude all generated code -->
<sonar.exclusions>**/generated-sources</sonar.exclusions>
- <plc4x-code-generation.version>1.4.0</plc4x-code-generation.version>
+ <plc4x-code-generation.version>1.5.0-SNAPSHOT</plc4x-code-generation.version>
Review comment:
uh are there some new upstream feature required?
##########
File path: build-utils/language-go/src/test/resources/plc4go/pom.xml
##########
@@ -0,0 +1,323 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review comment:
mostly this is for me a discussion reminder. (pom is pretty big, also plc4go received some updates in the meantime, artifactid should be something like example.com:example e.g.)
----------------------------------------------------------------
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