[GitHub] [arrow] wesm commented on pull request #7120: ARROW-8727: [C++] Don't require stack allocation of any object to use StringConverter, hide behind ParseValue function

2020-05-08 Thread GitBox 


wesm commented on pull request #7120:
URL: https://github.com/apache/arrow/pull/7120#issuecomment-626009344


   That’s fine. In general, introducing “nuisance” requirements for simple 
cases doesn’t seem like a good habit 



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:
us...@infra.apache.org

[GitHub] [arrow] wesm commented on pull request #7120: ARROW-8727: [C++] Don't require stack allocation of any object to use StringConverter, hide behind ParseValue function

2020-05-08 Thread GitBox 


wesm commented on pull request #7120:
URL: https://github.com/apache/arrow/pull/7120#issuecomment-625970866


   I left the `ParseValue` API as is and addressed the other comments. Since 
this is internal-only code would it be alright to call it a day on this? If 
someone wants to make more changes in a follow up PR that's fine with me



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:
us...@infra.apache.org

[GitHub] [arrow] wesm commented on pull request #7120: ARROW-8727: [C++] Don't require stack allocation of any object to use StringConverter, hide behind ParseValue function

2020-05-07 Thread GitBox 


wesm commented on pull request #7120:
URL: https://github.com/apache/arrow/pull/7120#issuecomment-625314666


   Done, PTAL



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:
us...@infra.apache.org

[GitHub] [arrow] wesm commented on pull request #7120: ARROW-8727: [C++] Don't require stack allocation of any object to use StringConverter, hide behind ParseValue function

2020-05-07 Thread GitBox 


wesm commented on pull request #7120:
URL: https://github.com/apache/arrow/pull/7120#issuecomment-625301501


   Well maybe:
   
   ```
   template 
   static inline bool ParseValue(const char* s, size_t length, typename 
T::c_type* out,
 const ParseContext* ctx = NULLPTR);
   ```



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:
us...@infra.apache.org

[GitHub] [arrow] wesm commented on pull request #7120: ARROW-8727: [C++] Don't require stack allocation of any object to use StringConverter, hide behind ParseValue function

2020-05-07 Thread GitBox 


wesm commented on pull request #7120:
URL: https://github.com/apache/arrow/pull/7120#issuecomment-625297750


   > For example, at some point conversion used C++ streams and a locale. The 
locale was stored on the converter instance.
   
   I'd prefer to handle this by passing a context/state argument into the 
`Parse` API so that the struct-initialization step is not required universally 
for all data types. 
   
   ```
   struct ConversionContext {};
   
   struct FloatConversionContext : public ConversionContext {
 std::locale ...;
   };
   
   ...
   
   ParseValue(&ctx, s, length, &out);
   ```



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:
us...@infra.apache.org