Java: fix unsigned default value code generation and add 'L' suffix for long default value #4051

blep · 2016-10-10T15:22:24Z

As mentionned in #4048 (keysearch_test), Java code generation is currently broken w.r.t. default value:

if the value type is unsigned and value is in the "unsigned range" (for example 255 for a ubyte)
if the value type is a long, the required 'L' suffix is missing

This PR fix those issue by "converting" unsigned value to their signed equivalent in Java (255 becomes -1), and adding 'L' suffix to long default value.

The full impact can be seen in one of our branch (where I integrated most pending PR):
https://github.com/rgilles/flatbuffers/blob/java_fieldbykey/tests/keysearch_test/Testing/KeySearch/UIntEntry.java#L19
Default value is -1 instead of 4294967295 as is currently generated:
https://github.com/rgilles/flatbuffers/blob/keysearch_test_data/tests/keysearch_test/Testing/KeySearch/UIntEntry.java#L19

… code and add "L" suffix to long default value.

ghost · 2016-10-10T16:35:18Z

I don't think I understand. We use "user facing" signed types one size bigger whenever you read an unsigned value. So ubyte is presented as a short. If a ubyte contains 255, I would want that to be 255 as a short, not -1, right?

blep · 2016-10-11T09:14:16Z

Hmm, indeed I suspect you are correct. The "signed" conversion should only be needed for ulong. Now that generated code compile, I can write some tests. I'll get back to you once I've done that.

…d specific handling as "user facing" type is int. uint need 'L' suffix as "user facing" type is long.

…buffers into java_unsigned_default_value # Conflicts: # tests/MyGame/Example/Monster.java

…er facing" type in builder.add<type>() calls.

ghost · 2016-10-12T22:39:45Z

tests/MyGame/Example/Monster.java

-  public static void addTesthashu32Fnv1(FlatBufferBuilder builder, long testhashu32Fnv1) { builder.addInt(17, (int)testhashu32Fnv1, 0); }
-  public static void addTesthashs64Fnv1(FlatBufferBuilder builder, long testhashs64Fnv1) { builder.addLong(18, testhashs64Fnv1, 0); }
-  public static void addTesthashu64Fnv1(FlatBufferBuilder builder, long testhashu64Fnv1) { builder.addLong(19, testhashu64Fnv1, 0); }
+  public static void addTesthashu32Fnv1(FlatBufferBuilder builder, long testhashu32Fnv1) { builder.addInt(17, (int)testhashu32Fnv1, (int)0L); }


here it is adding both the L and an (int) cast, that may be a bit superfluous?

As you asked, the code that generate the default value do it using user facing type. The problem is that some part of the builder API don't use user facing type, hence the cast.

ghost · 2016-10-12T22:40:30Z

tests/MyGame/Example/TestSimpleTableWithEnum.cs

@@ -26,7 +26,7 @@ public partial struct TestSimpleTableWithEnum : IFlatbufferObject
  }

  public static void StartTestSimpleTableWithEnum(FlatBufferBuilder builder) { builder.StartObject(1); }
-  public static void AddColor(FlatBufferBuilder builder, Color color) { builder.AddSbyte(0, (sbyte)color, 2); }
+  public static void AddColor(FlatBufferBuilder builder, Color color) { builder.AddSbyte(0, (sbyte)color, (sbyte)2); }


This may not be needed in C#

Good catch, should be fixed now.

googlebot · 2016-10-14T13:54:15Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

ghost · 2016-10-17T17:32:47Z

This PR gained a lot commits and changed files, something is not quite right?

googlebot · 2016-10-18T12:01:38Z

CLAs look good, thanks!

googlebot · 2016-10-18T12:12:18Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-10-18T12:33:35Z

CLAs look good, thanks!

blep · 2016-10-18T12:37:13Z

Fixed too many commit issues (I had merged romain's master by error instead of upstream).

ghost · 2016-10-19T18:29:22Z

src/idl_gen_general.cpp

-      code += ", " + GenDefaultValue(field.value, false);
+      code += ", ";
+      if (lang_.language == IDLOptions::kJava)
+        code += SourceCastBasic( field.value.type );


remove spaces in ()

ghost · 2016-10-19T18:30:50Z

Mostly looks good.. ready to go in?

blep · 2016-10-20T12:48:27Z

Yes, this has been well tested on my side with keysearch_test #4048 generated code.

ghost · 2016-10-21T21:27:22Z

Thanks!

blep added 2 commits October 7, 2016 18:06

Java: emit "signed" equivalent of unsigned default value in generated…

f9253bb

… code and add "L" suffix to long default value.

Updated generated code

885b789

blep added 3 commits October 11, 2016 13:18

Only convert ulong to "signed" equivalent. ubyte and ushort don't nee…

314ab63

…d specific handling as "user facing" type is int. uint need 'L' suffix as "user facing" type is long.

Merge branch 'java_unsigned_default_value' of github.com:rgilles/flat…

43c059d

…buffers into java_unsigned_default_value # Conflicts: # tests/MyGame/Example/Monster.java

Added missing cast to primitive type of default value which is in "us…

ec218a4

…er facing" type in builder.add<type>() calls.

ghost reviewed Oct 12, 2016

View reviewed changes

Do not cast default value to actual type in C#.

1945e11

blep force-pushed the java_unsigned_default_value branch from d89aded to 1945e11 Compare October 18, 2016 12:01

blep force-pushed the java_unsigned_default_value branch from d89aded to 1945e11 Compare October 18, 2016 12:33

ghost reviewed Oct 19, 2016

View reviewed changes

ghost merged commit 5b5fcbf into google:master Oct 21, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: fix unsigned default value code generation and add 'L' suffix for long default value #4051

Java: fix unsigned default value code generation and add 'L' suffix for long default value #4051

blep commented Oct 10, 2016

ghost commented Oct 10, 2016

blep commented Oct 11, 2016

ghost Oct 12, 2016

blep Oct 14, 2016

ghost Oct 12, 2016

blep Oct 14, 2016

googlebot commented Oct 14, 2016

ghost commented Oct 17, 2016

googlebot commented Oct 18, 2016

googlebot commented Oct 18, 2016

googlebot commented Oct 18, 2016

blep commented Oct 18, 2016

ghost Oct 19, 2016

ghost commented Oct 19, 2016

blep commented Oct 20, 2016

ghost commented Oct 21, 2016

Java: fix unsigned default value code generation and add 'L' suffix for long default value #4051

Java: fix unsigned default value code generation and add 'L' suffix for long default value #4051

Conversation

blep commented Oct 10, 2016

ghost commented Oct 10, 2016

blep commented Oct 11, 2016

ghost Oct 12, 2016

Choose a reason for hiding this comment

blep Oct 14, 2016

Choose a reason for hiding this comment

ghost Oct 12, 2016

Choose a reason for hiding this comment

blep Oct 14, 2016

Choose a reason for hiding this comment

googlebot commented Oct 14, 2016

ghost commented Oct 17, 2016

googlebot commented Oct 18, 2016

googlebot commented Oct 18, 2016

googlebot commented Oct 18, 2016

blep commented Oct 18, 2016

ghost Oct 19, 2016

Choose a reason for hiding this comment

ghost commented Oct 19, 2016

blep commented Oct 20, 2016

ghost commented Oct 21, 2016