[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

[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

[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] [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 =

[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