[GitHub] thrift pull request #1022: THRIFT-3845

[RobberPhex]

Github user RobberPhex closed the pull request at:

https://github.com/apache/thrift/pull/1022


---

[GitHub] thrift pull request #1022: THRIFT-3845

[nsuke]

Github user nsuke commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1022#discussion_r80386819
  
--- Diff: compiler/cpp/src/generate/t_php_generator.cc ---
@@ -1341,9 +1341,13 @@ void 
t_php_generator::generate_process_function(t_service* tservice, t_function*
 return;
   }
 
-  f_service_ << indent() << "$bin_accel = ($output instanceof "
- << "TBinaryProtocolAccelerated) && 
function_exists('thrift_protocol_write_binary');"
+  // for compatibility, add method_exists
+  f_service_ << indent() << "$bin_accel = method_exists($output, 
'isBinaryAccelerated')" << endl;
+  indent_up();
+  f_service_ << indent() << " && $output->isBinaryAccelerated()" << endl
+ << indent() << " && 
function_exists('thrift_protocol_write_binary');"
--- End diff --

The real problem may be that existing code generation here is too specific 
to one concrete type.
What if we move these thrift_protocol_..._binary calls to 
TBinaryProtocolAccelerated.php ?
That way we can move isStrict... checks there too so that we don't need 
them in TProtocolDecorator either.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request #1022: THRIFT-3845

[nsuke]

Github user nsuke commented on a diff in the pull request:

https://github.com/apache/thrift/pull/1022#discussion_r80386881
  
--- Diff: lib/php/lib/Thrift/Protocol/TBinaryProtocolAccelerated.php ---
@@ -62,4 +62,9 @@ public function isStrictWrite()
   {
 return $this->strictWrite_;
   }
+
+  public function isBinaryAccelerated()
+  {
+return true;
+  }
--- End diff --

I agree we need something like this but the name can be more general.
For example, `isAccelerated` or even `isDynamic`.
(Thinks about the day when we add TCompactProtocolAccelerated or anything 
without generated read/write calls...)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---