Oh sorry, I've misread the code. The variable is not returned as a value, instead it is used as an argument. Sorry again.
-----Original Message----- From: Taeyun Kim <taeyun....@innowireless.com> Sent: Friday, April 14, 2023 8:10 AM To: dev@arrow.apache.org Subject: RE: Probably an unnecessary copy when outputting join result? Hi, According to 'Effective Modern C++' book's item 25, you should not add std::move() to a local variable when returning it as a value. I think it would be better to check the book's content before applying std::move() to the code. Thank you. -----Original Message----- From: Sutou Kouhei <k...@clear-code.com> Sent: Friday, April 14, 2023 6:19 AM To: dev@arrow.apache.org Subject: Re: Probably an unnecessary copy when outputting join result? Hi, Could you re-read our "MINOR" definition? https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes > Any functionality change should have a GitHub issue opened. For minor > changes that affect documentation, you do not need to open up a GitHub > issue. Instead you can prefix the title of your PR with "MINOR: " if > meets the following guidelines: > > * Grammar, usage and spelling fixes that affect no more than 2 files > * Documentation updates affecting no more than 2 files and not more than 500 > words. See also a comment in our pull request template why we want to open a GitHub issue: https://raw.githubusercontent.com/apache/arrow/main/.github/pull_request_template.md > Opening GitHub issues ahead of time contributes to the Openness[*1] of > the Apache Arrow project. [*1]: https://theapacheway.com/open/ Thanks, -- kou In <6e182917-4a54-4a5c-8ccd-ac440cb8c...@gmail.com> "Re: Probably an unnecessary copy when outputting join result?" on Thu, 13 Apr 2023 10:57:59 -0700, Sasha Krassovsky <krassovskysa...@gmail.com> wrote: > Hi Rossi, > I think for small PRs like this it is fine to just prefix your PR with > “MINOR” and not have an associated issue. > > Sasha > >> On Apr 13, 2023, at 10:48 AM, Ruoxi Sun <zanmato1...@gmail.com> wrote: >> >> Hi Sasha, thanks for confirming. Wondering if I should file a github issue >> for this kind of trivial fix? >> >> Rossi >> >> >> Sasha Krassovsky <krassovskysa...@gmail.com >> <mailto:krassovskysa...@gmail.com>> 于2023年4月14日周五 01:44写道： >>> Hi Rossi, >>> That’s a good catch! I _think_ the compiler will automatically emit the >>> move because it sees we’re copying from an object that’ll never be used >>> again [1], but adding the std::move would be good just to remove any >>> ambiguity. Go ahead and make the PR! >>> >>> Sasha >>> >>> Move, simply >>> herbsutter.com >>> >>> <https://herbsutter.com/2020/02/17/move-simply/>Move, simply >>> <https://herbsutter.com/2020/02/17/move-simply/> >>> herbsutter.com <https://herbsutter.com/2020/02/17/move-simply/> >>> <https://herbsutter.com/2020/02/17/move-simply/> >>> >>>> 13 апр. 2023 г., в 10:17, Ruoxi Sun <zanmato1...@gmail.com >>>> <mailto:zanmato1...@gmail.com>> написал(а): >>>> >>>> Hi folks, when reading the swiss join code, I just noticed a small >>>> piece probably missing a `std::move()` call. >>>> >>>> See here: >>>> https://github.com/zanmato1984/arrow/commit/10f43c357db7a0287c642a2 >>>> 3e78027cb9cde6f25 >>>> >>>> If so, I think I can proceed to PR it. >>>> >>>> Thanks. >>>> >>>> *Rossi Sun* >
