Replace DM:Error with DM::Result.
Initially, Error was written with the intent that an empty string meant Ok and anything else meant Fatal. This made things simple with implicit constructors from strings. With the introduction of Nonfatal the state was now tied up with an additional boolean. Now the empty string meant Ok and causes the new boolean to be ignored, or at least that is the way it was used since Error didn't actually enforce that itself. This leads to GMs which return kSkip but don't set the message to not be skipped. This could be fixed in several ways. The first would be for the GMSrc to notice that a GM had returned kSkip with an empty message and create the Error::Nonfatal with a non-empty message. This has the downside of being some extra unexpected complexity and doesn't prevent similar issues from arising in the future. The second would be to change how DM interprets the Error, and if the non-fatal bit is set treat that as a sign to skip, even if the message is empty. This fixes the stated issue, but doesn't fix the issue where a GM can return kFail but also leave the message empty. This could again be fixed by either modifying GMSrc::draw or GM::drawContent, but this also seems a bit brittle in not preventing this from happening again in the future. So this replaces Error with Result, which makes the status orthogonal to the message. It does lose the automatic conversion from string, but by being able to wrap the many uses of SkStringPrintf the explicit nature doesn't add much additional noise to the more complex failure reports. Change-Id: Ibf48b67faa09a91a4a9d792d204bd9810b441c6c Reviewed-on: https://skia-review.googlesource.com/c/skia/+/270362 Commit-Queue: Ben Wagner <bungeman@google.com> Reviewed-by: Mike Klein <mtklein@google.com>
This commit is contained in:
Ben Wagner
Skia Commit-Bot
parent
7ef777efe1
commit
ea25fcf027
37
dm/DM.cpp
37
dm/DM.cpp
@ -902,9 +902,9 @@ static void push_sink(const SkCommandLineConfig& config, Sink* s) {
|
||||
|
||||
// Try a simple Src as a canary. If it fails, skip this sink.
|
||||
struct : public Src {
|
||||
Error draw(SkCanvas* c) const override {
|
||||
Result draw(SkCanvas* c) const override {
|
||||
c->drawRect(SkRect::MakeWH(1,1), SkPaint());
|
||||
return "";
|
||||
return Result::Ok();
|
||||
}
|
||||
SkISize size() const override { return SkISize::Make(16, 16); }
|
||||
Name name() const override { return "justOneRect"; }
|
||||
@ -913,9 +913,9 @@ static void push_sink(const SkCommandLineConfig& config, Sink* s) {
|
||||
SkBitmap bitmap;
|
||||
SkDynamicMemoryWStream stream;
|
||||
SkString log;
|
||||
Error err = sink->draw(justOneRect, &bitmap, &stream, &log);
|
||||
if (err.isFatal()) {
|
||||
info("Could not run %s: %s\n", config.getTag().c_str(), err.c_str());
|
||||
Result result = sink->draw(justOneRect, &bitmap, &stream, &log);
|
||||
if (result.isFatal()) {
|
||||
info("Could not run %s: %s\n", config.getTag().c_str(), result.c_str());
|
||||
exit(1);
|
||||
}
|
||||
|
||||
@ -1115,7 +1115,7 @@ struct Task {
|
||||
SkDynamicMemoryWStream stream;
|
||||
start(task.sink.tag.c_str(), task.src.tag.c_str(),
|
||||
task.src.options.c_str(), name.c_str());
|
||||
Error err = task.sink->draw(*task.src, &bitmap, &stream, &log);
|
||||
Result result = task.sink->draw(*task.src, &bitmap, &stream, &log);
|
||||
if (!log.isEmpty()) {
|
||||
info("%s %s %s %s:\n%s\n", task.sink.tag.c_str()
|
||||
, task.src.tag.c_str()
|
||||
@ -1123,19 +1123,18 @@ struct Task {
|
||||
, name.c_str()
|
||||
, log.c_str());
|
||||
}
|
||||
if (!err.isEmpty()) {
|
||||
if (err.isFatal()) {
|
||||
fail(SkStringPrintf("%s %s %s %s: %s",
|
||||
task.sink.tag.c_str(),
|
||||
task.src.tag.c_str(),
|
||||
task.src.options.c_str(),
|
||||
name.c_str(),
|
||||
err.c_str()));
|
||||
} else {
|
||||
done(task.sink.tag.c_str(), task.src.tag.c_str(),
|
||||
task.src.options.c_str(), name.c_str());
|
||||
return;
|
||||
}
|
||||
if (result.isSkip()) {
|
||||
done(task.sink.tag.c_str(), task.src.tag.c_str(),
|
||||
task.src.options.c_str(), name.c_str());
|
||||
return;
|
||||
}
|
||||
if (result.isFatal()) {
|
||||
fail(SkStringPrintf("%s %s %s %s: %s",
|
||||
task.sink.tag.c_str(),
|
||||
task.src.tag.c_str(),
|
||||
task.src.options.c_str(),
|
||||
name.c_str(),
|
||||
result.c_str()));
|
||||
}
|
||||
|
||||
// Run verifiers if specified
|
||||
|
||||
436
dm/DMSrcSink.cpp
436
dm/DMSrcSink.cpp
File diff suppressed because it is too large
Load Diff
111
dm/DMSrcSink.h
111
dm/DMSrcSink.h
@ -40,28 +40,41 @@ struct ImplicitString : public SkString {
|
||||
typedef ImplicitString Name;
|
||||
typedef ImplicitString Path;
|
||||
|
||||
class Error {
|
||||
class Result {
|
||||
public:
|
||||
Error(const SkString& s) : fMsg(s), fFatal(!this->isEmpty()) {}
|
||||
Error(const char* s) : fMsg(s), fFatal(!this->isEmpty()) {}
|
||||
enum class Status : int { Ok, Fatal, Skip };
|
||||
Result(Status status, const SkString& s) : fMsg(s), fStatus(status) {}
|
||||
Result(Status status, const char* s) : fMsg(s), fStatus(status) {}
|
||||
template <typename... Args> Result (Status status, const char* s, Args... args)
|
||||
: fMsg(SkStringPrintf(s, args...)), fStatus(status) {}
|
||||
|
||||
Error(const Error&) = default;
|
||||
Error& operator=(const Error&) = default;
|
||||
Result(const Result&) = default;
|
||||
Result& operator=(const Result&) = default;
|
||||
|
||||
static Error Nonfatal(const SkString& s) { return Nonfatal(s.c_str()); }
|
||||
static Error Nonfatal(const char* s) {
|
||||
Error e(s);
|
||||
e.fFatal = false;
|
||||
return e;
|
||||
static Result Ok() { return Result(Status::Ok, nullptr); }
|
||||
|
||||
static Result Fatal(const SkString& s) { return Result(Status::Fatal, s); }
|
||||
static Result Fatal(const char* s) { return Result(Status::Fatal, s); }
|
||||
template <typename... Args> static Result Fatal(const char* s, Args... args) {
|
||||
return Result(Status::Fatal, s, args...);
|
||||
}
|
||||
|
||||
static Result Skip(const SkString& s) { return Result(Status::Skip, s); }
|
||||
static Result Skip(const char* s) { return Result(Status::Skip, s); }
|
||||
template <typename... Args> static Result Skip(const char* s, Args... args) {
|
||||
return Result(Status::Skip, s, args...);
|
||||
}
|
||||
|
||||
bool isOk() { return fStatus == Status::Ok; }
|
||||
bool isFatal() { return fStatus == Status::Fatal; }
|
||||
bool isSkip() { return fStatus == Status::Skip; }
|
||||
|
||||
const char* c_str() const { return fMsg.c_str(); }
|
||||
bool isEmpty() const { return fMsg.isEmpty(); }
|
||||
bool isFatal() const { return fFatal; }
|
||||
Status status() const { return fStatus; }
|
||||
|
||||
private:
|
||||
SkString fMsg;
|
||||
bool fFatal;
|
||||
Status fStatus;
|
||||
};
|
||||
|
||||
struct SinkFlags {
|
||||
@ -74,14 +87,14 @@ struct SinkFlags {
|
||||
|
||||
struct Src {
|
||||
virtual ~Src() {}
|
||||
virtual Error SK_WARN_UNUSED_RESULT draw(SkCanvas*) const = 0;
|
||||
virtual Result SK_WARN_UNUSED_RESULT draw(SkCanvas*) const = 0;
|
||||
virtual SkISize size() const = 0;
|
||||
virtual Name name() const = 0;
|
||||
virtual void modifyGrContextOptions(GrContextOptions* options) const {}
|
||||
virtual bool veto(SinkFlags) const { return false; }
|
||||
|
||||
virtual int pageCount() const { return 1; }
|
||||
virtual Error SK_WARN_UNUSED_RESULT draw(int, SkCanvas* canvas) const {
|
||||
virtual Result SK_WARN_UNUSED_RESULT draw(int, SkCanvas* canvas) const {
|
||||
return this->draw(canvas);
|
||||
}
|
||||
virtual SkISize size(int) const { return this->size(); }
|
||||
@ -97,7 +110,7 @@ struct Src {
|
||||
struct Sink {
|
||||
virtual ~Sink() {}
|
||||
// You may write to either the bitmap or stream. If you write to log, we'll print that out.
|
||||
virtual Error SK_WARN_UNUSED_RESULT draw(const Src&, SkBitmap*, SkWStream*, SkString* log)
|
||||
virtual Result SK_WARN_UNUSED_RESULT draw(const Src&, SkBitmap*, SkWStream*, SkString* log)
|
||||
const = 0;
|
||||
|
||||
// Force Tasks using this Sink to run on the main thread?
|
||||
@ -118,7 +131,7 @@ class GMSrc : public Src {
|
||||
public:
|
||||
explicit GMSrc(skiagm::GMFactory);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
void modifyGrContextOptions(GrContextOptions* options) const override;
|
||||
@ -150,7 +163,7 @@ public:
|
||||
};
|
||||
CodecSrc(Path, Mode, DstColorType, SkAlphaType, float);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
bool veto(SinkFlags) const override;
|
||||
@ -168,7 +181,7 @@ class AndroidCodecSrc : public Src {
|
||||
public:
|
||||
AndroidCodecSrc(Path, CodecSrc::DstColorType, SkAlphaType, int sampleSize);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
bool veto(SinkFlags) const override;
|
||||
@ -196,7 +209,7 @@ public:
|
||||
|
||||
BRDSrc(Path, Mode, CodecSrc::DstColorType, uint32_t);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
bool veto(SinkFlags) const override;
|
||||
@ -215,7 +228,7 @@ public:
|
||||
};
|
||||
ImageGenSrc(Path, Mode, SkAlphaType, bool);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
bool veto(SinkFlags) const override;
|
||||
@ -232,7 +245,7 @@ class ColorCodecSrc : public Src {
|
||||
public:
|
||||
ColorCodecSrc(Path, bool decode_to_dst);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
bool veto(SinkFlags) const override;
|
||||
@ -245,7 +258,7 @@ class SKPSrc : public Src {
|
||||
public:
|
||||
explicit SKPSrc(Path path);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
private:
|
||||
@ -259,7 +272,7 @@ class BisectSrc : public SKPSrc {
|
||||
public:
|
||||
explicit BisectSrc(Path path, const char* trail);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
|
||||
private:
|
||||
SkString fTrail;
|
||||
@ -273,7 +286,7 @@ class SkottieSrc final : public Src {
|
||||
public:
|
||||
explicit SkottieSrc(Path path);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
bool veto(SinkFlags) const override;
|
||||
@ -301,7 +314,7 @@ class SVGSrc : public Src {
|
||||
public:
|
||||
explicit SVGSrc(Path path);
|
||||
|
||||
Error draw(SkCanvas*) const override;
|
||||
Result draw(SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
Name name() const override;
|
||||
bool veto(SinkFlags) const override;
|
||||
@ -321,8 +334,8 @@ public:
|
||||
explicit MSKPSrc(Path path);
|
||||
|
||||
int pageCount() const override;
|
||||
Error draw(SkCanvas* c) const override;
|
||||
Error draw(int, SkCanvas*) const override;
|
||||
Result draw(SkCanvas* c) const override;
|
||||
Result draw(int, SkCanvas*) const override;
|
||||
SkISize size() const override;
|
||||
SkISize size(int) const override;
|
||||
Name name() const override;
|
||||
@ -338,7 +351,7 @@ class NullSink : public Sink {
|
||||
public:
|
||||
NullSink() {}
|
||||
|
||||
Error draw(const Src& src, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src& src, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
const char* fileExtension() const override { return ""; }
|
||||
SinkFlags flags() const override { return SinkFlags{ SinkFlags::kNull, SinkFlags::kDirect }; }
|
||||
};
|
||||
@ -347,10 +360,10 @@ class GPUSink : public Sink {
|
||||
public:
|
||||
GPUSink(const SkCommandLineConfigGpu*, const GrContextOptions&);
|
||||
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Error onDraw(const Src&, SkBitmap*, SkWStream*, SkString*,
|
||||
const GrContextOptions& baseOptions,
|
||||
std::function<void(GrContext*)> initContext = nullptr) const;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result onDraw(const Src&, SkBitmap*, SkWStream*, SkString*,
|
||||
const GrContextOptions& baseOptions,
|
||||
std::function<void(GrContext*)> initContext = nullptr) const;
|
||||
|
||||
sk_gpu_test::GrContextFactory::ContextType contextType() const { return fContextType; }
|
||||
const sk_gpu_test::GrContextFactory::ContextOverrides& contextOverrides() {
|
||||
@ -387,7 +400,7 @@ class GPUThreadTestingSink : public GPUSink {
|
||||
public:
|
||||
GPUThreadTestingSink(const SkCommandLineConfigGpu*, const GrContextOptions&);
|
||||
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
|
||||
const char* fileExtension() const override {
|
||||
// Suppress writing out results from this config - we just want to do our matching test
|
||||
@ -404,7 +417,7 @@ class GPUPersistentCacheTestingSink : public GPUSink {
|
||||
public:
|
||||
GPUPersistentCacheTestingSink(const SkCommandLineConfigGpu*, const GrContextOptions&);
|
||||
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
|
||||
const char* fileExtension() const override {
|
||||
// Suppress writing out results from this config - we just want to do our matching test
|
||||
@ -421,7 +434,7 @@ class GPUPrecompileTestingSink : public GPUSink {
|
||||
public:
|
||||
GPUPrecompileTestingSink(const SkCommandLineConfigGpu*, const GrContextOptions&);
|
||||
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
|
||||
const char* fileExtension() const override {
|
||||
// Suppress writing out results from this config - we just want to do our matching test
|
||||
@ -435,7 +448,7 @@ private:
|
||||
class PDFSink : public Sink {
|
||||
public:
|
||||
PDFSink(bool pdfa, SkScalar rasterDpi) : fPDFA(pdfa), fRasterDpi(rasterDpi) {}
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
const char* fileExtension() const override { return "pdf"; }
|
||||
SinkFlags flags() const override { return SinkFlags{ SinkFlags::kVector, SinkFlags::kDirect }; }
|
||||
bool fPDFA;
|
||||
@ -446,7 +459,7 @@ class XPSSink : public Sink {
|
||||
public:
|
||||
XPSSink();
|
||||
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
const char* fileExtension() const override { return "xps"; }
|
||||
SinkFlags flags() const override { return SinkFlags{ SinkFlags::kVector, SinkFlags::kDirect }; }
|
||||
};
|
||||
@ -455,7 +468,7 @@ class RasterSink : public Sink {
|
||||
public:
|
||||
explicit RasterSink(SkColorType, sk_sp<SkColorSpace> = nullptr);
|
||||
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
const char* fileExtension() const override { return "png"; }
|
||||
SinkFlags flags() const override { return SinkFlags{ SinkFlags::kRaster, SinkFlags::kDirect }; }
|
||||
|
||||
@ -467,21 +480,21 @@ private:
|
||||
class ThreadedSink : public RasterSink {
|
||||
public:
|
||||
explicit ThreadedSink(SkColorType, sk_sp<SkColorSpace> = nullptr);
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
};
|
||||
|
||||
class SKPSink : public Sink {
|
||||
public:
|
||||
SKPSink();
|
||||
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
const char* fileExtension() const override { return "skp"; }
|
||||
SinkFlags flags() const override { return SinkFlags{ SinkFlags::kVector, SinkFlags::kDirect }; }
|
||||
};
|
||||
|
||||
class DebugSink : public Sink {
|
||||
public:
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
const char* fileExtension() const override { return "json"; }
|
||||
SinkFlags flags() const override { return SinkFlags{ SinkFlags::kVector, SinkFlags::kDirect }; }
|
||||
};
|
||||
@ -490,7 +503,7 @@ class SVGSink : public Sink {
|
||||
public:
|
||||
SVGSink(int pageIndex = 0);
|
||||
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
const char* fileExtension() const override { return "svg"; }
|
||||
SinkFlags flags() const override { return SinkFlags{ SinkFlags::kVector, SinkFlags::kDirect }; }
|
||||
|
||||
@ -518,7 +531,7 @@ protected:
|
||||
class ViaMatrix : public Via {
|
||||
public:
|
||||
ViaMatrix(SkMatrix, Sink*);
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
private:
|
||||
const SkMatrix fMatrix;
|
||||
};
|
||||
@ -526,7 +539,7 @@ private:
|
||||
class ViaUpright : public Via {
|
||||
public:
|
||||
ViaUpright(SkMatrix, Sink*);
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
private:
|
||||
const SkMatrix fMatrix;
|
||||
};
|
||||
@ -534,19 +547,19 @@ private:
|
||||
class ViaSerialization : public Via {
|
||||
public:
|
||||
explicit ViaSerialization(Sink* sink) : Via(sink) {}
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
};
|
||||
|
||||
class ViaPicture : public Via {
|
||||
public:
|
||||
explicit ViaPicture(Sink* sink) : Via(sink) {}
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
};
|
||||
|
||||
class ViaDDL : public Via {
|
||||
public:
|
||||
ViaDDL(int numReplays, int numDivisions, Sink* sink);
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
private:
|
||||
const int fNumReplays;
|
||||
const int fNumDivisions;
|
||||
@ -555,7 +568,7 @@ private:
|
||||
class ViaSVG : public Via {
|
||||
public:
|
||||
explicit ViaSVG(Sink* sink) : Via(sink) {}
|
||||
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
Result draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
|
||||
};
|
||||
|
||||
} // namespace DM
|
||||
|
||||
Loading…
Reference in New Issue
Block a user