Returning to this discussion. Here is my position on the matter since this was brought up on the sync call today
* For internal / non-public and pseudo-non-public APIs that have return/out values - Use Result or Status at discretion of the developer, but Result<T> is preferable * For new public APIs with return/out values - Prefer Result<T> unless a Status-based API seems definitely less awkward in real world use. I have to say that I'm skeptical about the relative usability of std::tuple outputs and don't think we should force the use of Result<T> for technical purity reasons * For existing Status APIs with return values - Incrementally add Result<T> APIs and deprecate Status-based APIs. Maintain deprecated Status APIs for ~2 major releases On Thu, Oct 24, 2019 at 5:16 PM Omer F. Ozarslan <o...@utdallas.edu> wrote: > > Hi Micah, > > You're right. Quite possible that clang-query counted same function > separately for each include in each file. (I was iterating each file > separately, but providing all of them at once didn't change the result > either.) > > It's cool and wrong, so not very useful apparently. :-) > > Best, > Omer > > On Thu, Oct 24, 2019 at 4:51 PM Micah Kornfield <emkornfi...@gmail.com> wrote: > > > > Hi Omer, > > I think this is really cool. It is quite possible it was underestimated (I > > agree about line lengths), but I think the clang query is double counting > > somehow. > > > > For instance: > > > > "grep -r Status *" only returns ~9000 results in total for me. > > > > Similarly using grep for "FinishTyped" returns 18 results for me. > > Searching through the log that you linked seems to return 450 (for "Status > > FinishTyped"). > > > > It is quite possible, I'm doing something naive with grep. > > > > Thanks, > > Micah > > > > On Thu, Oct 24, 2019 at 2:41 PM Omer F. Ozarslan <o...@utdallas.edu> wrote: > >> > >> Forgot to mention most of those lines are longer than line width while > >> out is usually (always?) last parameter, so probably that's why grep > >> possibly underestimates their number. > >> > >> On Thu, Oct 24, 2019 at 4:33 PM Omer F. Ozarslan <o...@utdallas.edu> wrote: > >> > > >> > Hi, > >> > > >> > I don't have much experience on customized clang-tidy plugins, but > >> > this might be a good use case for such a plugin from what I read here > >> > and there (frankly this was a good excuse for me to have a look at > >> > clang tooling as well). I wanted to ensure it isn't obviously overkill > >> > before this suggestion: Running a clang query which lists functions > >> > returning `arrow::Status` and taking a pointer parameter named `out` > >> > showed that there are 13947 such functions in `cpp/src/**/*.h`. [1] > >> > > >> > I checked logs and it seemed legitimate to me, but please check it in > >> > case I missed something. If that's the case, it might be tedious to do > >> > this work manually. > >> > > >> > [1]: https://gist.github.com/ozars/ecbb1b8acd4a57ba4721c1965f83f342 > >> > (Note that the log file is shown as truncated by github after ~30k > >> > lines) > >> > > >> > Best, > >> > Omer > >> > > >> > > >> > > >> > On Wed, Oct 23, 2019 at 9:23 PM Micah Kornfield <emkornfi...@gmail.com> > >> > wrote: > >> > > > >> > > OK, it sounds like people want Result<T> (at least in some > >> > > circumstances). > >> > > Any thoughts on migrating old APIs and what to do for new APIs going > >> > > forward? > >> > > > >> > > A very rough approximation [1] yields the following counts by module: > >> > > > >> > > 853 arrow > >> > > > >> > > 17 gandiva > >> > > > >> > > 25 parquet > >> > > > >> > > 50 plasma > >> > > > >> > > > >> > > > >> > > [1] grep -r Status cpp/src/* |grep ".h:" | grep "\\*" |grep -v Accept > >> > > |sed > >> > > s/:.*// | cut -f3 -d/ |sort > >> > > > >> > > > >> > > Thanks, > >> > > > >> > > Micah > >> > > > >> > > > >> > > > >> > > On Sat, Oct 19, 2019 at 7:50 PM Francois Saint-Jacques < > >> > > fsaintjacq...@gmail.com> wrote: > >> > > > >> > > > As mentioned, Result<T> is an improvement for function which returns > >> > > > a > >> > > > single value, e.g. Make/Factory-like. My vote goes Result<T> for such > >> > > > case. For multiple return types, we have std::tuple like Antoine > >> > > > proposed. > >> > > > > >> > > > François > >> > > > > >> > > > On Fri, Oct 18, 2019 at 9:19 PM Antoine Pitrou <anto...@python.org> > >> > > > wrote: > >> > > > > > >> > > > > > >> > > > > Le 18/10/2019 à 20:58, Wes McKinney a écrit : > >> > > > > > I'm definitely uncomfortable with the idea of deprecating Status. > >> > > > > > > >> > > > > > We have a few kinds of functions that can fail: > >> > > > > > > >> > > > > > 1. Functions with no "out" arguments > >> > > > > > 2. Functions with one out argument > >> > > > > > 3. Functions with multiple out arguments > >> > > > > > > >> > > > > > IMHO functions in category 2 are the best candidates for > >> > > > > > utilizing > >> > > > > > Status. In some cases, Case 3 may be more usable Result-based, > >> > > > > > but it > >> > > > > > can also create more work (or confusion) on the part of the > >> > > > > > developer, > >> > > > > > either > >> > > > > > > >> > > > > > * The T in Result<T> has to be a struct-like value that > >> > > > > > transports > >> > > > > > multiple pieces of data > >> > > > > > >> > > > > The T can be a std::tuple though, so you need not necessarily > >> > > > > define a > >> > > > > dedicated struct type for a single API's return value. > >> > > > > > >> > > > > > Can't say I'm thrilled about having Result<void> or similar for > >> > > > > Case > >> > > > > > 1-type functions (if I'm understanding what would be the > >> > > > > solution > >> > > > > > there). > >> > > > > > >> > > > > Agreed. > >> > > > > > >> > > > > Regards > >> > > > > > >> > > > > Antoine. > >> > > >
