jinq0123 / LuaPbIntf

Binding Protobuf 3 to Lua 5.3
MIT License
134 stars 41 forks source link

uint64 support broken #10

Closed seabadger closed 6 years ago

seabadger commented 6 years ago

given the following protobuf definition:

message MyMsg {
  uint64 uint64f = 1;
  int64  int64f  = 2;
}

and the following code:

   local msg = {
      uint64f = math.pow(2,32)
      int64f = math.pow(2,32)
   }
   local x =pb.decode("MyMsg", pb.encode( "MyMsg", msg ))
   print ( msg.int64f, msg.uint64f )
   print ( x.int64f, x.uint64f )

shows the following:

4294967296  4294967296
4294967296  0

Note that the int64 field is handled correctly, but uint64 is not. Using a value 2^32 causes the issue; 2^32-1 works as expected.

jinq0123 commented 6 years ago

My run result is:

4294967296.0    4294967296.0
4294967296      4294967296

And I added a test for uint64 which is OK.

seabadger commented 6 years ago

Thanks for the quick response. Looking a bit closer, there's one important thing I neglected to mention: I'm trying to use LuaPbIntf with Lua 5.1/5.2, and, as I just realized, this library's README only mentions 5.3...

In that case, I have a question: can this library be made to work reasonably with 5.2 and/or 5.1? In the snippet I provided, the signed int64 seems to work ok, only the unsigned uint64 seems to exhibit the issue, so I hope this is a good sign...

I realize that number representation changed from Lua5.2 and Lua5.3. 5.3 can represent 64-bit ints exactly, whereas 5.2 represents all numbers as doubles, and thus 64-bit ints > 2^53 lose precision; however, Lua 5.2 can represent integers <= 2^53 w/o an issue.

It'd be great if 64-bit integers in LuaPbIntf could work in a lua5.1/5.2 environment in the same way (i.e., exact up to a point, precision loss beyond that)

jinq0123 commented 6 years ago

Use proto double type instead of int64/uint64.

jinq0123 commented 6 years ago

For lua5.1/5.2, change

    case Fd::CPPTYPE_UINT64:
        return LuaRefValue(L, m_pRefl->GetUInt64(m_msg, &field));

to

    case Fd::CPPTYPE_UINT64:
        return LuaRefValue(L, double(m_pRefl->GetUInt64(m_msg, &field)));

That maybe work.

seabadger commented 6 years ago

Thanks. After doing it on both ends (in MsgToTbl and MessageSetter), it seems to have worked. The diff is below. It might be good to add the ability to enable this 'lua 5.1/5.2' behavior via a compile-time constant.

60c61,65
<         m_pRefl->SetUInt64(&m_rMsg, pField, luaValue.toValue<uint64>());
---
>         m_pRefl->SetUInt64(&m_rMsg, pField, luaValue.toValue<double>()); // VG

diff -r LuaPbIntf.Lua/src/detail/MsgToTbl.cpp LuaPbIntf/src/detail/MsgToTbl.cpp
<         return LuaRefValue(L, m_pRefl->GetUInt64(m_msg, &field));
---
>         return LuaRefValue(L, double(m_pRefl->GetUInt64(m_msg, &field))); // VG