AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Update REPORTER_ASSERT/ERRORF to check format strings.

Browse Source 
Previously, REPORTER_ASSERT/ERRORF relied on a helper function named
`reporter_string` which papered over zero-argument and one-argument
messages (where one-argument messages are assumed to ignore printf
formatting rules entirely, and just forward the message as-is).

Replacing this helper with a direct call to `SkStringPrintf` allows
the compiler to check format arguments for correctness, but sacrifices
the one-argument special case. In practice the one-argument special
case was very rarely used, so it's not a significant sacrifice,
and this did uncover several real errors in assertion format strings
(including some cases where the wrong number of arguments was passed).

Change-Id: I4378c43b16fd8fdbf4c78d849a9f2f0a254f7abc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/506617
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This commit is contained in:
John Stiles 2022-02-10 10:06:54 -05:00 committed by SkCQ
parent c5d3326d76
commit 2122f40c4b
14 changed files with 46 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
experimental/sktext/tests/UnicodeText.cpp
View File

@ -63,26 +63,26 @@ UNIX_ONLY_TEST(SkText_UnicodeText_Flags, reporter) {
auto lineBreak = utf16.find_first_of(u"\n");
for (size_t i = 0; i < unicodeText16.getText16().size(); ++i) {
if (i == lineBreak) {
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(i, CodeUnitFlags::kHardLineBreakBefore), "Pos16 %d should point to hard line break\n", lineBreak);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(i, CodeUnitFlags::kHardLineBreakBefore), "Pos8 %d should point to hard line break\n", lineBreak);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(i, CodeUnitFlags::kHardLineBreakBefore), "Pos16 %zu should point to hard line break\n", lineBreak);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(i, CodeUnitFlags::kHardLineBreakBefore), "Pos8 %zu should point to hard line break\n", lineBreak);
} else {
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(i, CodeUnitFlags::kGraphemeStart), "Pos16 %d should be a grapheme start\n", i);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(i, CodeUnitFlags::kGraphemeStart), "Pos8 %d should be a grapheme start\n", i);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(i, CodeUnitFlags::kGraphemeStart), "Pos16 %zu should be a grapheme start\n", i);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(i, CodeUnitFlags::kGraphemeStart), "Pos8 %zu should be a grapheme start\n", i);
}
}
auto space1 = utf16.find_first_of(u" ");
auto space2 = utf16.find_last_of(u" ");
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(space1, CodeUnitFlags::kPartOfWhiteSpace), "Pos16 %d should be a part of whitespaces\n", space1);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(space1 + 1, CodeUnitFlags::kSoftLineBreakBefore), "Pos16 %d should have soft line break before\n", space1 + 1);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(space2, CodeUnitFlags::kPartOfWhiteSpace), "Pos16 %d should be a part of whitespaces\n", space2);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(space2 + 1, CodeUnitFlags::kSoftLineBreakBefore), "Pos16 %d should have soft line break before\n", space2 + 1);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(space1, CodeUnitFlags::kPartOfWhiteSpace), "Pos16 %zu should be a part of whitespaces\n", space1);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(space1 + 1, CodeUnitFlags::kSoftLineBreakBefore), "Pos16 %zu should have soft line break before\n", space1 + 1);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(space2, CodeUnitFlags::kPartOfWhiteSpace), "Pos16 %zu should be a part of whitespaces\n", space2);
REPORTER_ASSERT(reporter, unicodeText16.hasProperty(space2 + 1, CodeUnitFlags::kSoftLineBreakBefore), "Pos16 %zu should have soft line break before\n", space2 + 1);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(space1, CodeUnitFlags::kPartOfWhiteSpace), "Pos8 %d should be a part of whitespaces\n", space1);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(space1 + 1, CodeUnitFlags::kSoftLineBreakBefore), "Pos8 %d should have soft line break before\n", space1 + 1);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(space2, CodeUnitFlags::kPartOfWhiteSpace), "Pos8 %d should be a part of whitespaces\n", space2);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(space2 + 1, CodeUnitFlags::kSoftLineBreakBefore), "Pos8 %d should have soft line break before\n", space2 + 1);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(space1, CodeUnitFlags::kPartOfWhiteSpace), "Pos8 %zu should be a part of whitespaces\n", space1);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(space1 + 1, CodeUnitFlags::kSoftLineBreakBefore), "Pos8 %zu should have soft line break before\n", space1 + 1);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(space2, CodeUnitFlags::kPartOfWhiteSpace), "Pos8 %zu should be a part of whitespaces\n", space2);
REPORTER_ASSERT(reporter, unicodeText8 .hasProperty(space2 + 1, CodeUnitFlags::kSoftLineBreakBefore), "Pos8 %zu should have soft line break before\n", space2 + 1);
}
// TODO: Test RTL text

4
modules/skparagraph/tests/SkParagraphTest.cpp
View File

@ -2041,13 +2041,13 @@ UNIX_ONLY_TEST(SkParagraph_JustifyRTLNewLine, reporter) {
// All lines should be justified to the width of the paragraph
// except for #0 (new line) and #5 (the last one)
for (auto& line : impl->lines()) {
auto num = &line - impl->lines().data();
ptrdiff_t num = &line - impl->lines().data();
if (num == 0 || num == 5) {
REPORTER_ASSERT(reporter, line.width() < TestCanvasWidth - 100);
} else {
REPORTER_ASSERT(reporter,
SkScalarNearlyEqual(line.width(), TestCanvasWidth - 100, EPSILON100),
"#%d: %f <= %f\n", num, line.width(), TestCanvasWidth - 100);
"#%zd: %f <= %d\n", num, line.width(), TestCanvasWidth - 100);
}
}
}

2
tests/AnimatedImageTest.cpp
View File

@ -76,7 +76,7 @@ DEF_TEST(AnimatedImage_rotation, r) {
auto bounds = animatedImage->getBounds();
if (bounds != expectedBounds) {
ERRORF(r, "Mismatched bounds for %", file);
ERRORF(r, "Mismatched bounds for %s", file);
bounds.dump();
}
}

7
tests/BackendAllocationTest.cpp
View File

@ -52,7 +52,7 @@ void test_wrapping(GrDirectContext* dContext,
sk_sp<ManagedBackendTexture> mbet = create(dContext, mipMapped, renderable);
if (!mbet) {
ERRORF(reporter, "Couldn't create backendTexture for grColorType %d renderable %s\n",
grColorType,
(int)grColorType,
GrRenderable::kYes == renderable ? "yes" : "no");
return;
}
@ -285,7 +285,8 @@ static void check_base_readbacks(GrDirectContext* dContext,
GrColorInfo info(colorType, kUnpremul_SkAlphaType, nullptr);
auto surfaceContext = dContext->priv().makeSC(readView, info);
if (!surfaceContext) {
ERRORF(reporter, "Could not create surface context for colorType: %d\n", colorType);
ERRORF(reporter, "Could not create surface context for colorType: %d\n",
(int)colorType);
}
if (!surfaceContext->readPixels(dContext, actual, {0, 0})) {
@ -293,7 +294,7 @@ static void check_base_readbacks(GrDirectContext* dContext,
// arbitrary colorType
#if 0
ERRORF(reporter, "Couldn't readback from SurfaceContext for colorType: %d\n",
colorType);
(int)colorType);
#endif
} else {
auto name = SkStringPrintf("%s::readPixels",

2
tests/CodecTest.cpp
View File

@ -1683,7 +1683,7 @@ DEF_TEST(Codec_webp_rowsDecoded, r) {
sk_sp<SkData> subset = SkData::MakeSubset(data.get(), 0, truncatedSize);
std::unique_ptr<SkCodec> codec = SkCodec::MakeFromData(std::move(subset));
if (!codec) {
ERRORF(r, "Failed to create a codec for %s truncated to only %lu bytes",
ERRORF(r, "Failed to create a codec for %s truncated to only %zu bytes",
path, truncatedSize);
return;
}

2
tests/EmptyPathTest.cpp
View File

@ -49,7 +49,7 @@ static void drawAndTest(skiatest::Reporter* reporter, const SkPath& path,
ERRORF(reporter, "%s style[%d] cap[%d] join[%d] antialias[%d]"
" filltype[%d] ptcount[%d]", str, paint.getStyle(),
paint.getStrokeCap(), paint.getStrokeJoin(),
paint.isAntiAlias(), path.getFillType(), path.countPoints());
paint.isAntiAlias(), (int)path.getFillType(), path.countPoints());
// uncomment this if you want to step in to see the failure
// canvas.drawPath(path, p);
}

7
tests/GrClipStackTest.cpp
View File

@ -379,7 +379,7 @@ void TestCase::run(const std::vector<int>& order,
matchedElements += found ? 1 : 0;
}
REPORTER_ASSERT(reporter, matchedElements == fExpectedElements.size(),
"%s, did not match all expected elements: expected %d but matched only %d",
"%s, did not match all expected elements: expected %zu but matched only %zu",
name.c_str(), fExpectedElements.size(), matchedElements);
// Validate restoration behavior
@ -388,7 +388,8 @@ void TestCase::run(const std::vector<int>& order,
cs.restore();
REPORTER_ASSERT(reporter, cs.clipState() == oldState,
"%s, restoring an empty save record should not change clip state: "
"expected %d but got %d", (int) oldState, (int) cs.clipState());
"expected %d but got %d",
name.c_str(), (int) oldState, (int) cs.clipState());
} else if (policy != SavePolicy::kNever) {
int restoreCount = policy == SavePolicy::kAtStart ? 1 : (int) order.size();
for (int i = 0; i < restoreCount; ++i) {
@ -397,7 +398,7 @@ void TestCase::run(const std::vector<int>& order,
// Should be wide open if everything is restored to base state
REPORTER_ASSERT(reporter, cs.clipState() == ClipStack::ClipState::kWideOpen,
"%s, restore should make stack become wide-open, not %d",
(int) cs.clipState());
name.c_str(), (int) cs.clipState());
}
}

6
tests/ImageFilterTest.cpp
View File

@ -490,8 +490,8 @@ static void test_cropRects(skiatest::Reporter* reporter, GrRecordingContext* rCo
SkImageFilter_Base::Context ctx(SkMatrix::I(), SkIRect::MakeWH(100, 100), nullptr,
kN32_SkColorType, nullptr, srcImg.get());
sk_sp<SkSpecialImage> resultImg(as_IFB(filter)->filterImage(ctx).imageAndOffset(&offset));
REPORTER_ASSERT(reporter, resultImg, filters.getName(i));
REPORTER_ASSERT(reporter, offset.fX == 20 && offset.fY == 30, filters.getName(i));
REPORTER_ASSERT(reporter, resultImg, "%s", filters.getName(i));
REPORTER_ASSERT(reporter, offset.fX == 20 && offset.fY == 30, "%s", filters.getName(i));
}
}
@ -809,7 +809,7 @@ DEF_TEST(ImageFilterDrawTiled, reporter) {
}
if (!ToolUtils::equal_pixels(untiledResult, tiledResult)) {
REPORTER_ASSERT(reporter, false, filters.getName(i));
ERRORF(reporter, "%s", filters.getName(i));
break;
}
}

6
tests/ProcessorTest.cpp
View File

@ -740,13 +740,13 @@ DEF_GPUTEST_FOR_GL_RENDERING_CONTEXTS(ProcessorOptimizationValidationTest, repor
// Print first failing pixel's details.
if (!coverageMessage.isEmpty()) {
ERRORF(reporter, coverageMessage.c_str());
ERRORF(reporter, "%s", coverageMessage.c_str());
}
if (!constMessage.isEmpty()) {
ERRORF(reporter, constMessage.c_str());
ERRORF(reporter, "%s", constMessage.c_str());
}
if (!opaqueMessage.isEmpty()) {
ERRORF(reporter, opaqueMessage.c_str());
ERRORF(reporter, "%s", opaqueMessage.c_str());
}
if (!loggedFirstFailure) {

2
tests/RecordingXfermodeTest.cpp
View File

@ -171,6 +171,6 @@ DEF_TEST(SkRecordingAccuracyXfermode, reporter) {
}
#if !FINEGRAIN
REPORTER_ASSERT(reporter, 0 == numErrors, errors.c_str());
REPORTER_ASSERT(reporter, 0 == numErrors, "%s", errors.c_str());
#endif
}

2
tests/Skbug6653.cpp
View File

@ -90,7 +90,7 @@ static void test_bug_6653(GrDirectContext* dContext,
}
}
REPORTER_ASSERT(reporter, match, label);
REPORTER_ASSERT(reporter, match, "%s", label);
}
}

4
tests/StreamTest.cpp
View File

@ -506,7 +506,7 @@ DEF_TEST(FILEStreamWithOffset, r) {
const size_t size = stream1.getLength();
const size_t middle = size / 2;
if (!stream1.seek(middle)) {
ERRORF(r, "Could not seek SkFILEStream to %lu out of %lu", middle, size);
ERRORF(r, "Could not seek SkFILEStream to %zu out of %zu", middle, size);
return;
}
REPORTER_ASSERT(r, stream1.getPosition() == middle);
@ -518,7 +518,7 @@ DEF_TEST(FILEStreamWithOffset, r) {
}
if (fseek(file, (long) middle, SEEK_SET) != 0) {
ERRORF(r, "Could not fseek FILE to %lu out of %lu", middle, size);
ERRORF(r, "Could not fseek FILE to %zu out of %zu", middle, size);
return;
}
SkFILEStream stream2(file);

24
tests/Test.h
View File

@ -165,24 +165,16 @@ private:
} // namespace skiatest
static inline SkString reporter_string() { return {}; }
/// Prevent security warnings when using a non-literal string i.e. not a format string.
static inline SkString reporter_string(const char* s) { return SkString(s); }
template<typename... Args>
static inline SkString reporter_string(const char* fmt, Args... args) {
return SkStringPrintf(fmt, std::forward<Args>(args)...);
}
#define REPORTER_ASSERT(r, cond, ...) \
do { \
if (!(cond)) { \
REPORT_FAILURE(r, #cond, reporter_string(__VA_ARGS__)); \
} \
#define REPORTER_ASSERT(r, cond, ...) \
do { \
if (!(cond)) { \
REPORT_FAILURE(r, #cond, SkStringPrintf(__VA_ARGS__)); \
} \
} while (0)
#define ERRORF(r, ...) \
do { \
REPORT_FAILURE(r, "", reporter_string(__VA_ARGS__)); \
#define ERRORF(r, ...) \
do { \
REPORT_FAILURE(r, "", SkStringPrintf(__VA_ARGS__)); \
} while (0)
#define INFOF(REPORTER, ...) \

6
tests/VkYcbcrSamplerTest.cpp
View File

@ -99,17 +99,17 @@ DEF_GPUTEST(VkYCbcrSampler_DrawImageWithYcbcrSampler, reporter, options) {
int r = readbackData[(y * kImageWidth + x) * 4];
if (abs(r - expectedR) > kColorTolerance) {
ERRORF(reporter, "R should be %d, but is %d at (%d, %d)", expectedR, r, x, y);
ERRORF(reporter, "R should be %d, but is %d at (%zu, %zu)", expectedR, r, x, y);
}
int g = readbackData[(y * kImageWidth + x) * 4 + 1];
if (abs(g - expectedG) > kColorTolerance) {
ERRORF(reporter, "G should be %d, but is %d at (%d, %d)", expectedG, g, x, y);
ERRORF(reporter, "G should be %d, but is %d at (%zu, %zu)", expectedG, g, x, y);
}
int b = readbackData[(y * kImageWidth + x) * 4 + 2];
if (abs(b - expectedB) > kColorTolerance) {
ERRORF(reporter, "B should be %d, but is %d at (%d, %d)", expectedB, b, x, y);
ERRORF(reporter, "B should be %d, but is %d at (%zu, %zu)", expectedB, b, x, y);
}
}
}