From 6ad9909fb7ccc973f35bbd222b0f424b8c3df0d2 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Wed, 26 Oct 2016 10:35:22 -0400 Subject: [PATCH] Turn on -Wrange-loop-analysis. -Wrange-loop-analysis triggers when we use a new-style for loop in a way that appears to unintentionally call a copy constructor on each non-trivial loop element instead of operating on them by reference. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=4000 Change-Id: If9e1b7fcc1f2789ae03c41c17abb17e60d564a8b Reviewed-on: https://skia-review.googlesource.com/4000 Reviewed-by: Ethan Nicholas Commit-Queue: Mike Klein --- gn/BUILD.gn | 1 - src/sksl/SkSLCompiler.cpp | 42 +++++++++++++++++++++--------------------- tests/FontMgrTest.cpp | 2 +- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/gn/BUILD.gn b/gn/BUILD.gn index 8451b9f..cf092a2 100644 --- a/gn/BUILD.gn +++ b/gn/BUILD.gn @@ -284,7 +284,6 @@ config("warnings") { ] cflags_cc += [ "-Wno-abstract-vbase-init", - "-Wno-range-loop-analysis", "-Wno-weak-vtables", ] diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp index 5b502dc..c3adaea 100644 --- a/src/sksl/SkSLCompiler.cpp +++ b/src/sksl/SkSLCompiler.cpp @@ -4,7 +4,7 @@ * Use of this source code is governed by a BSD-style license that can be * found in the LICENSE file. */ - + #include "SkSLCompiler.h" #include @@ -41,7 +41,7 @@ static const char* SKSL_FRAG_INCLUDE = namespace SkSL { -Compiler::Compiler() +Compiler::Compiler() : fErrorCount(0) { auto types = std::shared_ptr(new SymbolTable(*this)); auto symbols = std::shared_ptr(new SymbolTable(types, *this)); @@ -159,23 +159,23 @@ void Compiler::addDefinition(const Expression* lvalue, const Expression* expr, // We consider the variable written to as long as at least some of its components have // been written to. This will lead to some false negatives (we won't catch it if you // write to foo.x and then read foo.y), but being stricter could lead to false positives - // (we write to foo.x, and then pass foo to a function which happens to only read foo.x, - // but since we pass foo as a whole it is flagged as an error) unless we perform a much + // (we write to foo.x, and then pass foo to a function which happens to only read foo.x, + // but since we pass foo as a whole it is flagged as an error) unless we perform a much // more complicated whole-program analysis. This is probably good enough. - this->addDefinition(((Swizzle*) lvalue)->fBase.get(), - fContext.fDefined_Expression.get(), + this->addDefinition(((Swizzle*) lvalue)->fBase.get(), + fContext.fDefined_Expression.get(), definitions); break; case Expression::kIndex_Kind: // see comments in Swizzle - this->addDefinition(((IndexExpression*) lvalue)->fBase.get(), - fContext.fDefined_Expression.get(), + this->addDefinition(((IndexExpression*) lvalue)->fBase.get(), + fContext.fDefined_Expression.get(), definitions); break; case Expression::kFieldAccess_Kind: // see comments in Swizzle - this->addDefinition(((FieldAccess*) lvalue)->fBase.get(), - fContext.fDefined_Expression.get(), + this->addDefinition(((FieldAccess*) lvalue)->fBase.get(), + fContext.fDefined_Expression.get(), definitions); break; default: @@ -185,7 +185,7 @@ void Compiler::addDefinition(const Expression* lvalue, const Expression* expr, } // add local variables defined by this node to the set -void Compiler::addDefinitions(const BasicBlock::Node& node, +void Compiler::addDefinitions(const BasicBlock::Node& node, std::unordered_map* definitions) { switch (node.fKind) { case BasicBlock::Node::kExpression_Kind: { @@ -249,15 +249,15 @@ void Compiler::scanCFG(CFG* cfg, BlockId blockId, std::set* workList) { // is initially unknown static std::unordered_map compute_start_state(const CFG& cfg) { std::unordered_map result; - for (const auto block : cfg.fBlocks) { - for (const auto node : block.fNodes) { + for (const auto& block : cfg.fBlocks) { + for (const auto& node : block.fNodes) { if (node.fKind == BasicBlock::Node::kStatement_Kind) { const Statement* s = (Statement*) node.fNode; if (s->fKind == Statement::kVarDeclarations_Kind) { const VarDeclarationsStatement* vd = (const VarDeclarationsStatement*) s; for (const VarDeclaration& decl : vd->fDeclaration->fVars) { result[decl.fVar] = nullptr; - } + } } } } @@ -282,7 +282,7 @@ void Compiler::scanCFG(const FunctionDefinition& f) { // check for unreachable code for (size_t i = 0; i < cfg.fBlocks.size(); i++) { - if (i != cfg.fStart && !cfg.fBlocks[i].fEntrances.size() && + if (i != cfg.fStart && !cfg.fBlocks[i].fEntrances.size() && cfg.fBlocks[i].fNodes.size()) { this->error(cfg.fBlocks[i].fNodes[0].fNode->fPosition, "unreachable"); } @@ -299,11 +299,11 @@ void Compiler::scanCFG(const FunctionDefinition& f) { const Expression* expr = (const Expression*) n.fNode; if (expr->fKind == Expression::kVariableReference_Kind) { const Variable& var = ((VariableReference*) expr)->fVariable; - if (var.fStorage == Variable::kLocal_Storage && + if (var.fStorage == Variable::kLocal_Storage && !definitions[&var]) { this->error(expr->fPosition, "'" + var.fName + "' has not been assigned"); - } + } } } this->addDefinitions(n, &definitions); @@ -332,7 +332,7 @@ void Compiler::internalConvertProgram(std::string text, switch (decl.fKind) { case ASTDeclaration::kVar_Kind: { std::unique_ptr s = fIRGenerator->convertVarDeclarations( - (ASTVarDeclarations&) decl, + (ASTVarDeclarations&) decl, Variable::kGlobal_Storage); if (s) { result->push_back(std::move(s)); @@ -398,7 +398,7 @@ std::unique_ptr Compiler::convertProgram(Program::Kind kind, std::strin fIRGenerator->fSymbolTable->markAllFunctionsBuiltin(); Modifiers::Flag defaultPrecision; this->internalConvertProgram(text, &defaultPrecision, &elements); - auto result = std::unique_ptr(new Program(kind, defaultPrecision, std::move(elements), + auto result = std::unique_ptr(new Program(kind, defaultPrecision, std::move(elements), fIRGenerator->fSymbolTable)); fIRGenerator->popSymbolTable(); this->writeErrorCount(); @@ -444,7 +444,7 @@ bool Compiler::toSPIRV(Program::Kind kind, const std::string& text, std::string* return result; } -bool Compiler::toGLSL(Program::Kind kind, const std::string& text, GLCaps caps, +bool Compiler::toGLSL(Program::Kind kind, const std::string& text, GLCaps caps, std::ostream& out) { auto program = this->convertProgram(kind, text); if (fErrorCount == 0) { @@ -455,7 +455,7 @@ bool Compiler::toGLSL(Program::Kind kind, const std::string& text, GLCaps caps, return fErrorCount == 0; } -bool Compiler::toGLSL(Program::Kind kind, const std::string& text, GLCaps caps, +bool Compiler::toGLSL(Program::Kind kind, const std::string& text, GLCaps caps, std::string* out) { std::stringstream buffer; bool result = this->toGLSL(kind, text, caps, buffer); diff --git a/tests/FontMgrTest.cpp b/tests/FontMgrTest.cpp index c8c8b94..172bff3 100644 --- a/tests/FontMgrTest.cpp +++ b/tests/FontMgrTest.cpp @@ -690,7 +690,7 @@ static void test_matchStyleCSS3(skiatest::Reporter* reporter) { }; for (StyleSetTest& test : tests) { - for (const StyleSetTest::Case testCase : test.cases) { + for (const StyleSetTest::Case& testCase : test.cases) { SkAutoTUnref typeface(test.styleSet.matchStyle(testCase.pattern)); if (typeface) { REPORTER_ASSERT(reporter, typeface->fontStyle() == testCase.expectedResult); -- 2.7.4