[GitHub] thrift issue #1052: THRIFT-3885 PHP: Error when readI64 in TCompactProtocol

2016-07-23 Thread nsuke
Github user nsuke commented on the issue: https://github.com/apache/thrift/pull/1052 Also I noticed that `if (true)` part below inside the existing function seems completely redundant. `else` code even uses different set of variables. We can clean it up without losing any value

[GitHub] thrift issue #1052: THRIFT-3885 PHP: Error when readI64 in TCompactProtocol

2016-07-23 Thread nsuke
Github user nsuke commented on the issue: https://github.com/apache/thrift/pull/1052 I'm inclined to make it stupid simple... ``` php // Shift hi and lo together. if ($shift < 28) { $lo |= (($byte & 0x7f) << $shift); } elseif ($shift == 28) { $lo |= (($byte &

[GitHub] thrift issue #1052: THRIFT-3885 PHP: Error when readI64 in TCompactProtocol

2016-07-23 Thread czm1989
Github user czm1989 commented on the issue: https://github.com/apache/thrift/pull/1052 @nsuke Yes, I agree. It also can simply be `$hi |= (($byte & 0x7f) >> 4);` because of the shift is only 28 that satisfy the condition. But according to the algorithm of writeVarint, I think

[GitHub] thrift issue #1052: THRIFT-3885 PHP: Error when readI64 in TCompactProtocol

2016-07-23 Thread nsuke
Github user nsuke commented on the issue: https://github.com/apache/thrift/pull/1052 I believe this is duplicate of THRIFT-3827 (#1008) and the correct fix is there too. As PHP cross test has been useless, I'm verifying the fix in THRIFT-3886 (#1054). --- If your project is set