This CL has a complicated back story, but it's concrete change is
simple, just turning the warning on and converting a bunch of
return foo;
to
return std::move(foo);
These changes are exclusively in places where RVO and NRVO do not apply,
so it should not conflict with warnings like -Wpessimizing-move.
Since C++11, when you return a named local and its type doesn't match
the declared return type exactly, there's an implicit std::move()
wrapped around the value (what I'm making explicit here) so the move
constructor gets an opportunity to take precedence over the copy
constructor. You can read about this implicit move here under the
section "automatic move from local variables and parameters":
https://en.cppreference.com/w/cpp/language/return#Notes.
This situation comes up for us with smart pointers: a function declares
its return type as std::unique_ptr<Base> or sk_sp<Base>, and we return a
std::unique_ptr<Impl> or sk_sp<Impl>. Those types don't match exactly,
so RVO and NRVO don't come into play. They've always been going through
move constructors, and that's not changed here, just made explicit.
There was apparently once a bug in the C++11 standard and compilers
implementing that which made these copy instead of move, and then this
sort of code would do a little unnecessary ref/unref dance for sk_sp,
and would entirely fail to compile for uncopyable std::unique_ptr.
These explicit moves ostensibly will make our code more compatible with
those older compilers.
That compatibility alone is, I think, a terrible reason to land this CL.
Like, actively bad. But... to balance that out, I think the explicit
std::move()s here actually help remind us that RVO/NRVO are not in play,
and remind us we're going to call the move constructor. So that C++11
standard bug becomes kind of useful for us, in that Clang added this
warning to catch it, and its fix improves readability.
So really read this all as, "warn about implicit std::move() on return".
In the end I think it's just about readability. I don't really hold any
hope out that we'll become compatible with those older compilers.
Bug: skia:9909
Change-Id: Id596e9261188b6f10e759906af6c95fe303f6ffe
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/271601
Reviewed-by: Brian Salomon <bsalomon@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
These unused comparison operators are the only users of
<functional> in SkRefCnt.h, for std::less. <functional>
is an expensive header to compile, and SkRefCnt.h is popular,
so it helps to cut dependencies like this.
Mostly we just need to add #include <functional> in a few
places that were picking it up via SkRefCnt.h.
In SkPixmapPriv.h, it looked simpler to template the argument,
since everything was inline anyway.
Change-Id: I7c125bb26a04199847357c729a1b178256c6ef8d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/236942
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Change-Id: I127c979670c3dc7dac2e35908a795afbdefca8f6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/234902
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Current strategy: everything from the top
Things to look at first are the manual changes:
- added tools/rewrite_includes.py
- removed -Idirectives from BUILD.gn
- various compile.sh simplifications
- tweak tools/embed_resources.py
- update gn/find_headers.py to write paths from the top
- update gn/gn_to_bp.py SkUserConfig.h layout
so that #include "include/config/SkUserConfig.h" always
gets the header we want.
No-Presubmit: true
Change-Id: I73a4b181654e0e38d229bc456c0d0854bae3363e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/209706
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Hal Canary <halcanary@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Florin Malita <fmalita@chromium.org>
- add drop-in clang-tidy cxx wrapper
- get build clean for bugprone-use-after-move
The wrapper can be used by setting
cxx = "/path/to/skia/tools/clang-tidy.sh"
in GN.
Change-Id: Idbba911e23bd6ef7530b08fd31906b92c1c1b28c
Reviewed-on: https://skia-review.googlesource.com/c/176523
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Change-Id: I871dd5eea4496e87c206b46d9eae81cb521b11ce
Reviewed-on: https://skia-review.googlesource.com/65103
Reviewed-by: Hal Canary <halcanary@google.com>
Commit-Queue: Mike Klein <mtklein@chromium.org>
BUG=skia:
Change-Id: I8df5c84bf961a3be68a642ba338fdc641e09df59
Reviewed-on: https://skia-review.googlesource.com/8889
Reviewed-by: Mike Klein <mtklein@chromium.org>
Commit-Queue: Brian Osman <brianosman@google.com>
This enables removing the more complicated atomic shims from SkAtomics.h.
TBR=reed
This doesn't actually change any API.
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-TSAN-Trybot,Test-Ubuntu-GCC-Golo-GPU-GT610-x86_64-Release-TSAN-Trybot
Review URL: https://codereview.chromium.org/1867863002
The 'element_type' typedef is to play nice with std::pointer_traits.
The full complement of operators and swap to match unique_ptr so that
sk_sp can be properly compared to nullptr and used with standard
containers.
Update to 'reset' so that calling 'unref' is the last operation.
This also adds tests for these changes, and sets the fPtr to nullptr
in debug for easier bug finding.
Review URL: https://codereview.chromium.org/1773453002
Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>.
But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl().
Making ~Effect() virtual fixes this.
This should make our Google3 tests sized-delete clean, unblocking those folks.
Review URL: https://codereview.chromium.org/1769093002
Previously, sk_sp::reset(T* t) did not release its own reference
if its internal pointer was the same as 't'. This leaks a reference.
Now always release the current reference when non-nullptr.
Review URL: https://codereview.chromium.org/1767983002
It's only used by a couple unit tests. We have other ways of getting
the same quality testing of our ref-count code now (e.g. TSAN).
BUG=skia:
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac10.9-Clang-x86_64-Release-CMake-Trybot
Review URL: https://codereview.chromium.org/1408213005
