Skip to content

Commit

Permalink
Fix all warnings in engine_vulkan_debugger
Browse files Browse the repository at this point in the history
Remove unused functions.
Add explicit casts.
Avoid variable shadowing.
Be explicit with virtual `override`s
Use epsilons for float compares.
Move VkDebugger to anonymous namespace, as it isn't referenced outside
the `engine_vulkan_debugger` compilation unit.
Remove pointless semi-colons.
  • Loading branch information
ben-clayton committed Jul 16, 2020
1 parent 298b24a commit 64c0373
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 34 deletions.
2 changes: 0 additions & 2 deletions src/vulkan/engine_vulkan.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ namespace vulkan {
/// Engine implementation based on Vulkan.
class EngineVulkan : public Engine {
public:
class VkDebugger;

EngineVulkan();
~EngineVulkan() override;

Expand Down
57 changes: 25 additions & 32 deletions src/vulkan/engine_vulkan_debugger.cc
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,6 @@ static constexpr const auto kStepEventTimeout = std::chrono::seconds(1);
// Event provides a basic wait-and-signal synchronization primitive.
class Event {
public:
// Wait blocks until the event is fired.
void Wait() {
std::unique_lock<std::mutex> lock(mutex_);
cv_.wait(lock, [&] { return signalled_; });
}

// Wait blocks until the event is fired, or the timeout is reached.
// If the Event was signalled, then Wait returns true, otherwise false.
template <typename Rep, typename Period>
Expand Down Expand Up @@ -143,11 +137,6 @@ struct Variable {

// Get parses the Variable value for the requested type, assigning the result
// to |out|. Returns true on success, otherwise false.
bool Get(int* out) const {
*out = std::atoi(value.c_str());
return true; // TODO(bclayton): Verify the value parsed correctly.
}

bool Get(uint32_t* out) const {
*out = static_cast<uint32_t>(std::atoi(value.c_str()));
return true; // TODO(bclayton): Verify the value parsed correctly.
Expand All @@ -158,11 +147,6 @@ struct Variable {
return true; // TODO(bclayton): Verify the value parsed correctly.
}

bool Get(float* out) const {
*out = std::atof(value.c_str());
return true; // TODO(bclayton): Verify the value parsed correctly.
}

bool Get(double* out) const {
*out = std::atof(value.c_str());
return true; // TODO(bclayton): Verify the value parsed correctly.
Expand Down Expand Up @@ -259,7 +243,7 @@ class Client {
const dap::StackFrame& frame,
debug::Location* location,
std::string* line = nullptr) {
location->line = frame.line;
location->line = static_cast<uint32_t>(frame.line);

if (!frame.source.has_value()) {
onerror_("Stack frame with name '" + frame.name + "' has no source");
Expand Down Expand Up @@ -507,8 +491,8 @@ InvocationKey::InvocationKey(const WindowSpacePosition& pos)
data.window_space_position = pos;
}

InvocationKey::InvocationKey(Type type, const Data& data)
: type(type), data(data) {}
InvocationKey::InvocationKey(Type type_, const Data& data_)
: type(type_), data(data_) {}

std::string InvocationKey::String() const {
std::stringstream ss;
Expand Down Expand Up @@ -564,7 +548,7 @@ class Thread : public debug::Thread {
});
}

~Thread() { Flush(); }
~Thread() override { Flush(); }

// Flush waits for the debugger thread script to complete, and returns any
// errors encountered.
Expand Down Expand Up @@ -707,21 +691,32 @@ class Thread : public debug::Thread {

void ExpectLocal(const std::string& name, int64_t value) override {
DEBUGGER_LOG("ExpectLocal('%s', %d)", name.c_str(), (int)value);
ExpectLocalT(name, value);
ExpectLocalT(name, value, [](int64_t a, int64_t b) { return a == b; });
}

void ExpectLocal(const std::string& name, double value) override {
DEBUGGER_LOG("ExpectLocal('%s', %f)", name.c_str(), value);
ExpectLocalT(name, value);
ExpectLocalT(name, value,
[](double a, double b) { return std::abs(a - b) < 0.00001; });
}

void ExpectLocal(const std::string& name, const std::string& value) override {
DEBUGGER_LOG("ExpectLocal('%s', '%s')", name.c_str(), value.c_str());
ExpectLocalT(name, value);
ExpectLocalT(name, value, [](const std::string& a, const std::string& b) {
return a == b;
});
}

template <typename T>
void ExpectLocalT(const std::string& name, const T& expect) {
// ExpectLocalT verifies that the local variable with the given name has the
// expected value. |name| may contain `.` delimiters to index structure or
// array types. ExpectLocalT uses the function equal to compare the expected
// and actual values, returning true if considered equal, otherwise false.
// EQUAL_FUNC is a function-like type with the signature:
// bool(T a, T b)
template <typename T, typename EQUAL_FUNC>
void ExpectLocalT(const std::string& name,
const T& expect,
EQUAL_FUNC&& equal) {
dap::StackFrame frame;
if (!client_.TopStackFrame(thread_id_, &frame)) {
return;
Expand Down Expand Up @@ -758,7 +753,7 @@ class Thread : public debug::Thread {
return;
}

if (got != expect) {
if (!equal(got, expect)) {
std::stringstream ss;
ss << "Local '" << name << "' did not have expected value. Value is '"
<< got << "', expected '" << expect << "'";
Expand Down Expand Up @@ -805,11 +800,8 @@ class Thread : public debug::Thread {
Result error_;
};

} // namespace

// EngineVulkan::VkDebugger is a private implementation of the Engine::Debugger
// interface.
class EngineVulkan::VkDebugger : public Engine::Debugger {
// VkDebugger is a private implementation of the Engine::Debugger interface.
class VkDebugger : public Engine::Debugger {
static constexpr const char* kComputeShaderFunctionName = "ComputeShader";
static constexpr const char* kVertexShaderFunctionName = "VertexShader";
static constexpr const char* kFragmentShaderFunctionName = "FragmentShader";
Expand Down Expand Up @@ -927,7 +919,7 @@ class EngineVulkan::VkDebugger : public Engine::Debugger {
const std::shared_ptr<const debug::ThreadScript>& script) override {
std::unique_lock<std::mutex> lock(threads_mutex_);
pendingThreads_.emplace(GlobalInvocationId{x, y, z}, script);
};
}

void BreakOnVertexIndex(
uint32_t index,
Expand Down Expand Up @@ -1140,6 +1132,7 @@ class EngineVulkan::VkDebugger : public Engine::Debugger {
std::mutex error_mutex_;
Result error_; // guarded by error_mutex_
};
} // namespace

std::pair<Engine::Debugger*, Result> EngineVulkan::GetDebugger(
VirtualFileStore* virtual_files) {
Expand Down

0 comments on commit 64c0373

Please sign in to comment.