[llvm] r284069 - [unittests] Delete even more copy constructors (NFC)

[David Blaikie via llvm-commits]

On Mon, Oct 17, 2016 at 11:25 AM Vedant Kumar <vsk at apple.com> wrote:

> > If the move ctor/assign is noexcept, it should be OK as-is (without
> removing the copy ops).
>
> Thanks for sharing this, I did not know about this facet of 'noexcept'.
>
> > I'd probably avoid the extra complexity & leaev the code as it was
> before.
>
> I'd like to keep the {Input,Output}FunctionCoverageData classes consistent
> in
> whether/not they are copyable. Since I'd also prefer for the move
> constructor
> to kick in during, e.g vector::resize, do you think the cleanest solution
> would
> be to make both classes copyable but mark the move methods 'LLVM_NOEXCEPT'?
>

Really if you have to write them down - you're in about the same place as
you are today. (though side note: you can just write the move ops as you
have already, and omit the copy ops - they'll be implicitly omitted)

In theory they should be noexcept already if everything they depend on is
noexcept - so the right fix is probably to make sure the underlying data
structures are noexcept (maybe they can't be? - SmallVector might have to
copy on move (if it's in the small mode), etc... )


>
> vedant
>
> > On Oct 17, 2016, at 11:07 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, Oct 17, 2016 at 11:02 AM Vedant Kumar <vsk at apple.com> wrote:
> > While we didn't have any cases where we'd copy InputFunctionCoverageData
> > objects, I wanted to prevent these copies from ever occurring because
> they
> > would be expensive. Each object contains a SmallDenseMap<unsigned,
> unsigned>
> > and a std::vector<CounterMappingRegion>.
> >
> > This commit was motivated by a case where we called std::vector::resize
> on a
> > vector of OutputFunctionCoverageData. Deleting the copy constructor in
> that
> > case should have made the resize operation cheaper (caveat - I haven't
> actually
> > measured this).
> >
> > If the move ctor/assign is noexcept, it should be OK as-is (without
> removing the copy ops). We (C++/Clang/etc) don't really have a good answer
> for this in a codebase without exceptions anyway... :/
> >
> > This is pretty far outside any of the code I deal with, so do whatever
> works best for you, but I'd probably avoid the extra complexity & leaev the
> code as it was before.
> >
> > - Dave
> >
> >
> > vedant
> >
> > > On Oct 17, 2016, at 9:58 AM, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > > Were these copies problematic in any way? If it's just a POD type, you
> might as well leave it copyable, etc?
> > >
> > > On Wed, Oct 12, 2016 at 3:53 PM Vedant Kumar via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> > > Author: vedantk
> > > Date: Wed Oct 12 17:44:50 2016
> > > New Revision: 284069
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=284069&view=rev
> > > Log:
> > > [unittests] Delete even more copy constructors (NFC)
> > >
> > > Modified:
> > >     llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
> > >
> > > Modified: llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp?rev=284069&r1=284068&r2=284069&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp (original)
> > > +++ llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp Wed Oct
> 12 17:44:50 2016
> > > @@ -106,6 +106,16 @@ struct InputFunctionCoverageData {
> > >
> > >    InputFunctionCoverageData(std::string Name, uint64_t Hash)
> > >        : Name(std::move(Name)), Hash(Hash) {}
> > > +
> > > +  InputFunctionCoverageData(InputFunctionCoverageData &&IFCD)
> > > +      :
> ReverseVirtualFileMapping(std::move(IFCD.ReverseVirtualFileMapping)),
> > > +        Name(std::move(IFCD.Name)), Hash(IFCD.Hash),
> > > +        Regions(std::move(IFCD.Regions)) {}
> > > +
> > > +  InputFunctionCoverageData(const InputFunctionCoverageData &) =
> delete;
> > > +  InputFunctionCoverageData &
> > > +  operator=(const InputFunctionCoverageData &) = delete;
> > > +  InputFunctionCoverageData &operator=(InputFunctionCoverageData &&)
> = delete;
> > >  };
> > >
> > >  struct CoverageMappingTest : ::testing::Test {
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161017/5b8b2950/attachment.html>