arrow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Omer F. Ozarslan" <o...@utdallas.edu>
Subject Re: [DISCUSS] Result vs Status
Date Thu, 24 Oct 2019 22:05:31 GMT
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 <emkornfield@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 <omer@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 <omer@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 <emkornfield@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 <
>> > > fsaintjacques@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 <antoine@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.
>> > > >

Mime
View raw message