turn on -Wreturn-std-move-in-c++11
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>
diff --git a/gn/BUILD.gn b/gn/BUILD.gn
index 0d703f6..fde8064 100644
--- a/gn/BUILD.gn
+++ b/gn/BUILD.gn
@@ -438,7 +438,6 @@
"-Wno-c++98-compat",
"-Wno-c++98-compat-pedantic",
"-Wno-undefined-func-template",
- "-Wno-return-std-move-in-c++11",
]
cflags_objc += [
"-Wno-direct-ivar-access",
diff --git a/modules/skottie/src/SkottiePriv.h b/modules/skottie/src/SkottiePriv.h
index c07144f..e6d5e6b 100644
--- a/modules/skottie/src/SkottiePriv.h
+++ b/modules/skottie/src/SkottiePriv.h
@@ -121,7 +121,7 @@
auto node = adapter->node();
this->attachDiscardableAdapter(std::move(adapter));
- return node;
+ return std::move(node);
}
return nullptr;
diff --git a/src/core/SkColorFilter.cpp b/src/core/SkColorFilter.cpp
index edd5869..38cd478 100644
--- a/src/core/SkColorFilter.cpp
+++ b/src/core/SkColorFilter.cpp
@@ -419,7 +419,7 @@
}
fp->addChild(std::move(childFP));
}
- return fp;
+ return std::move(fp);
}
#endif
diff --git a/src/core/SkMiniRecorder.cpp b/src/core/SkMiniRecorder.cpp
index 76919a0..db11f99 100644
--- a/src/core/SkMiniRecorder.cpp
+++ b/src/core/SkMiniRecorder.cpp
@@ -102,7 +102,7 @@
auto pic = sk_make_sp<SkMiniPicture<T>>(cull, std::move(*op)); \
op->~T(); \
fState = State::kEmpty; \
- return pic; \
+ return std::move(pic); \
}
static SkOnce once;
diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp
index cc250d5..4d4edf5 100644
--- a/src/core/SkRemoteGlyphCache.cpp
+++ b/src/core/SkRemoteGlyphCache.cpp
@@ -1015,5 +1015,5 @@
wire.typefaceID, wire.glyphCount, wire.style, wire.isFixed,
fDiscardableHandleManager, fIsLogging);
fRemoteFontIdToTypeface.set(wire.typefaceID, newTypeface);
- return newTypeface;
+ return std::move(newTypeface);
}
diff --git a/src/core/SkStream.cpp b/src/core/SkStream.cpp
index bb53e2c..ad627b9 100644
--- a/src/core/SkStream.cpp
+++ b/src/core/SkStream.cpp
@@ -903,7 +903,7 @@
if (!stream->isValid()) {
return nullptr;
}
- return stream;
+ return std::move(stream);
}
// Declared in SkStreamPriv.h:
diff --git a/src/gpu/GrCopyRenderTask.cpp b/src/gpu/GrCopyRenderTask.cpp
index 39fa454..0f46e69 100644
--- a/src/gpu/GrCopyRenderTask.cpp
+++ b/src/gpu/GrCopyRenderTask.cpp
@@ -42,7 +42,7 @@
sk_sp<GrCopyRenderTask> task(new GrCopyRenderTask(
std::move(srcView), clippedSrcRect, std::move(dstView), clippedDstPoint));
- return task;
+ return std::move(task);
}
GrCopyRenderTask::GrCopyRenderTask(GrSurfaceProxyView srcView,
diff --git a/src/gpu/GrResourceProvider.cpp b/src/gpu/GrResourceProvider.cpp
index 1833748..f239f0a 100644
--- a/src/gpu/GrResourceProvider.cpp
+++ b/src/gpu/GrResourceProvider.cpp
@@ -349,7 +349,7 @@
const void* data,
const GrUniqueKey& key) {
if (auto buffer = this->findByUniqueKey<GrGpuBuffer>(key)) {
- return buffer;
+ return std::move(buffer);
}
if (auto buffer = this->createBuffer(size, intendedType, kStatic_GrAccessPattern, data)) {
// We shouldn't bin and/or cache static buffers.
@@ -397,7 +397,7 @@
SkASSERT(key->isValid());
this->assignUniqueKeyToResource(*key, buffer.get());
}
- return buffer;
+ return std::move(buffer);
}
///////////////////////////////////////////////////////////////////////////////////////////////////
diff --git a/src/gpu/gl/GrGLAssembleGLESInterfaceAutogen.cpp b/src/gpu/gl/GrGLAssembleGLESInterfaceAutogen.cpp
index 22e70cd..f9361a6 100644
--- a/src/gpu/gl/GrGLAssembleGLESInterfaceAutogen.cpp
+++ b/src/gpu/gl/GrGLAssembleGLESInterfaceAutogen.cpp
@@ -522,6 +522,6 @@
interface->fStandard = kGLES_GrGLStandard;
interface->fExtensions.swap(&extensions);
- return interface;
+ return std::move(interface);
}
#endif
diff --git a/src/gpu/gl/GrGLAssembleGLInterfaceAutogen.cpp b/src/gpu/gl/GrGLAssembleGLInterfaceAutogen.cpp
index 21fb2b4..e100f78 100644
--- a/src/gpu/gl/GrGLAssembleGLInterfaceAutogen.cpp
+++ b/src/gpu/gl/GrGLAssembleGLInterfaceAutogen.cpp
@@ -514,6 +514,6 @@
interface->fStandard = kGL_GrGLStandard;
interface->fExtensions.swap(&extensions);
- return interface;
+ return std::move(interface);
}
#endif
diff --git a/src/gpu/gl/GrGLAssembleWebGLInterfaceAutogen.cpp b/src/gpu/gl/GrGLAssembleWebGLInterfaceAutogen.cpp
index e48e525..163a382 100644
--- a/src/gpu/gl/GrGLAssembleWebGLInterfaceAutogen.cpp
+++ b/src/gpu/gl/GrGLAssembleWebGLInterfaceAutogen.cpp
@@ -239,6 +239,6 @@
interface->fStandard = kWebGL_GrGLStandard;
interface->fExtensions.swap(&extensions);
- return interface;
+ return std::move(interface);
}
#endif
diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index ee69933..c7092a2 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -706,7 +706,7 @@
backendTex.getGLTextureParams(), cacheable, ioType);
// We don't know what parameters are already set on wrapped textures.
texture->textureParamsModified();
- return texture;
+ return std::move(texture);
}
static bool check_compressed_backend_texture(const GrBackendTexture& backendTex,
@@ -759,7 +759,7 @@
kRead_GrIOType);
// We don't know what parameters are already set on wrapped textures.
texture->textureParamsModified();
- return texture;
+ return std::move(texture);
}
sk_sp<GrTexture> GrGLGpu::onWrapRenderableBackendTexture(const GrBackendTexture& backendTex,
@@ -805,7 +805,7 @@
texRT->baseLevelWasBoundToFBO();
// We don't know what parameters are already set on wrapped textures.
texRT->textureParamsModified();
- return texRT;
+ return std::move(texRT);
}
sk_sp<GrRenderTarget> GrGLGpu::onWrapBackendRenderTarget(const GrBackendRenderTarget& backendRT,
@@ -1393,7 +1393,7 @@
}
}
}
- return tex;
+ return std::move(tex);
}
sk_sp<GrTexture> GrGLGpu::onCreateCompressedTexture(SkISize dimensions,
@@ -1430,7 +1430,7 @@
// The non-sampler params are still at their default values.
tex->parameters()->set(&initialState, GrGLTextureParameters::NonsamplerState(),
fResetTimestampForTextureParameters);
- return tex;
+ return std::move(tex);
}
GrBackendTexture GrGLGpu::onCreateCompressedBackendTexture(SkISize dimensions,
diff --git a/src/gpu/mtl/GrMtlGpu.mm b/src/gpu/mtl/GrMtlGpu.mm
index cbcdf94..4a86ff4 100644
--- a/src/gpu/mtl/GrMtlGpu.mm
+++ b/src/gpu/mtl/GrMtlGpu.mm
@@ -497,7 +497,7 @@
levelClearMask);
}
- return tex;
+ return std::move(tex);
}
sk_sp<GrTexture> GrMtlGpu::onCreateCompressedTexture(SkISize dimensions,
@@ -608,7 +608,7 @@
[transferBuffer didModifyRange: NSMakeRange(bufferOffset, dataSize)];
#endif
- return tex;
+ return std::move(tex);
}
static id<MTLTexture> get_texture_from_backend(const GrBackendTexture& backendTex) {
diff --git a/src/gpu/vk/GrVkGpu.cpp b/src/gpu/vk/GrVkGpu.cpp
index 856b3ea..0215406 100644
--- a/src/gpu/vk/GrVkGpu.cpp
+++ b/src/gpu/vk/GrVkGpu.cpp
@@ -148,7 +148,7 @@
!vkGpu->vkCaps().supportsProtectedMemory()) {
return nullptr;
}
- return vkGpu;
+ return std::move(vkGpu);
}
////////////////////////////////////////////////////////////////////////////////
@@ -1072,7 +1072,7 @@
this->currentCommandBuffer()->clearColorImage(this, tex.get(), &kZeroClearColor,
ranges.count(), ranges.begin());
}
- return tex;
+ return std::move(tex);
}
sk_sp<GrTexture> GrVkGpu::onCreateCompressedTexture(SkISize dimensions,
@@ -1126,7 +1126,7 @@
return nullptr;
}
- return tex;
+ return std::move(tex);
}
////////////////////////////////////////////////////////////////////////////////
@@ -1334,7 +1334,7 @@
SkASSERT(tgt->canAttemptStencilAttachment());
}
- return tgt;
+ return std::move(tgt);
}
sk_sp<GrRenderTarget> GrVkGpu::onWrapBackendTextureAsRenderTarget(const GrBackendTexture& tex,
diff --git a/src/lazy/SkDiscardableMemoryPool.cpp b/src/lazy/SkDiscardableMemoryPool.cpp
index e147586..987c3d1 100644
--- a/src/lazy/SkDiscardableMemoryPool.cpp
+++ b/src/lazy/SkDiscardableMemoryPool.cpp
@@ -172,7 +172,7 @@
fList.addToHead(dm.get());
fUsed += bytes;
this->dumpDownTo(fBudget);
- return dm;
+ return std::move(dm);
}
void DiscardableMemoryPool::removeFromPool(PoolDiscardableMemory* dm) {
diff --git a/src/pdf/SkPDFMetadata.cpp b/src/pdf/SkPDFMetadata.cpp
index ca0a825..6019457 100644
--- a/src/pdf/SkPDFMetadata.cpp
+++ b/src/pdf/SkPDFMetadata.cpp
@@ -132,7 +132,7 @@
if (metadata.fModified != kZeroTime) {
dict->insertString("ModDate", pdf_date(metadata.fModified));
}
- return dict;
+ return std::move(dict);
}
SkUUID SkPDFMetadata::CreateUUID(const SkPDF::Metadata& metadata) {
@@ -176,7 +176,7 @@
SkString(reinterpret_cast<const char*>(&doc), sizeof(SkUUID)));
array->appendString(
SkString(reinterpret_cast<const char*>(&instance), sizeof(SkUUID)));
- return array;
+ return std::move(array);
}
// Convert a block of memory to hexadecimal. Input and output pointers will be
diff --git a/src/ports/SkFontHost_mac.cpp b/src/ports/SkFontHost_mac.cpp
index b9bdb72..655180b 100644
--- a/src/ports/SkFontHost_mac.cpp
+++ b/src/ports/SkFontHost_mac.cpp
@@ -2194,7 +2194,7 @@
CFDictionaryAddValue(ctVariation.get(), axisTag, axisValue);
}
- return ctVariation;
+ return std::move(ctVariation);
}
int SkTypeface_Mac::onGetVariationDesignPosition(
diff --git a/src/shaders/SkRTShader.cpp b/src/shaders/SkRTShader.cpp
index 3e961e0..f966cc7 100644
--- a/src/shaders/SkRTShader.cpp
+++ b/src/shaders/SkRTShader.cpp
@@ -150,7 +150,7 @@
if (GrColorTypeClampType(args.fDstColorInfo->colorType()) != GrClampType::kNone) {
return GrFragmentProcessor::ClampPremulOutput(std::move(fp));
} else {
- return fp;
+ return std::move(fp);
}
}
#endif
diff --git a/src/utils/SkCanvasStateUtils.cpp b/src/utils/SkCanvasStateUtils.cpp
index 73fb5ca..59526d0 100644
--- a/src/utils/SkCanvasStateUtils.cpp
+++ b/src/utils/SkCanvasStateUtils.cpp
@@ -304,7 +304,7 @@
state_v1->layers[i].y));
}
- return canvas;
+ return std::move(canvas);
}
////////////////////////////////////////////////////////////////////////////////
diff --git a/tests/RefCntTest.cpp b/tests/RefCntTest.cpp
index 6c23249..c19cc8c 100644
--- a/tests/RefCntTest.cpp
+++ b/tests/RefCntTest.cpp
@@ -138,7 +138,7 @@
static sk_sp<Effect> make_effect() {
auto foo = EffectImpl::Create();
foo->fValue = 42;
- return foo;
+ return std::move(foo);
}
static void reset_counters() {
diff --git a/tests/SRGBReadWritePixelsTest.cpp b/tests/SRGBReadWritePixelsTest.cpp
index 14a90f1..5e85e01 100644
--- a/tests/SRGBReadWritePixelsTest.cpp
+++ b/tests/SRGBReadWritePixelsTest.cpp
@@ -197,7 +197,7 @@
if (!surfaceContext) {
ERRORF(reporter, "Could not create %s surface context.", encoding_as_str(contextEncoding));
}
- return surfaceContext;
+ return std::move(surfaceContext);
}
static void test_write_read(Encoding contextEncoding, Encoding writeEncoding, Encoding readEncoding,
diff --git a/tools/gpu/gl/interface/templates.go b/tools/gpu/gl/interface/templates.go
index 650e855..ebed908 100644
--- a/tools/gpu/gl/interface/templates.go
+++ b/tools/gpu/gl/interface/templates.go
@@ -76,7 +76,7 @@
interface->fStandard = kGLES_GrGLStandard;
interface->fExtensions.swap(&extensions);
- return interface;
+ return std::move(interface);
}
#endif
`
@@ -143,7 +143,7 @@
interface->fStandard = kGL_GrGLStandard;
interface->fExtensions.swap(&extensions);
- return interface;
+ return std::move(interface);
}
#endif
`
@@ -208,7 +208,7 @@
interface->fStandard = kWebGL_GrGLStandard;
interface->fExtensions.swap(&extensions);
- return interface;
+ return std::move(interface);
}
#endif
`
