From a2e94a5ac3bf09e0258a89069ec43088a1b424b2 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Thu, 21 Apr 2022 20:48:11 +0000 Subject: [PATCH] Fix readability/todo issues. This CL adds names to the TODOs which did not have them and enables the readability/todo lint. The names are based on the git blame for the TODO lines. Bug: dawn:1339 Change-Id: I25a2920bc8fa9606f5dda67a629fdef1c10f8948 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/87600 Reviewed-by: Austin Eng Commit-Queue: Dan Sinclair --- src/dawn/CPPLINT.cfg | 1 - src/dawn/common/StackContainer.h | 2 -- src/dawn/common/SystemUtils.cpp | 2 +- src/dawn/native/d3d12/RenderPassBuilderD3D12.cpp | 16 +++------------- src/dawn/native/metal/RenderPipelineMTL.mm | 4 ++-- src/dawn/native/metal/TextureMTL.mm | 4 +--- src/dawn/native/vulkan/CommandBufferVk.cpp | 1 - src/dawn/native/vulkan/SwapChainVk.cpp | 5 +++-- src/dawn/node/binding/GPU.cpp | 2 +- src/dawn/node/binding/GPUBuffer.cpp | 4 ++-- src/dawn/samples/ManualSwapChainTest.cpp | 2 +- src/dawn/tests/end2end/DeviceLostTests.cpp | 4 ++-- src/dawn/tests/end2end/SamplerTests.cpp | 1 - .../tests/end2end/TextureSubresourceTests.cpp | 8 -------- src/dawn/tests/end2end/VertexStateTests.cpp | 8 -------- .../validation/ResourceUsageTrackingTests.cpp | 7 ------- .../validation/TextureSubresourceTests.cpp | 8 -------- 17 files changed, 16 insertions(+), 63 deletions(-) diff --git a/src/dawn/CPPLINT.cfg b/src/dawn/CPPLINT.cfg index a62541f5c5..f5c9c6dfc4 100644 --- a/src/dawn/CPPLINT.cfg +++ b/src/dawn/CPPLINT.cfg @@ -1,2 +1 @@ -filter=-readability/todo filter=-runtime/indentation_namespace diff --git a/src/dawn/common/StackContainer.h b/src/dawn/common/StackContainer.h index db0bf91599..f531261ec4 100644 --- a/src/dawn/common/StackContainer.h +++ b/src/dawn/common/StackContainer.h @@ -83,8 +83,6 @@ class StackAllocator : public std::allocator { // For this constructor, we cannot share storage; there's // no guarantee that the Source buffer of Ts is large enough // for Us. - // TODO: If we were fancy pants, perhaps we could share storage - // iff sizeof(T) == sizeof(U). template StackAllocator(const StackAllocator& other) : source_(nullptr) { } diff --git a/src/dawn/common/SystemUtils.cpp b/src/dawn/common/SystemUtils.cpp index a5ce0f1540..a6385ad7d0 100644 --- a/src/dawn/common/SystemUtils.cpp +++ b/src/dawn/common/SystemUtils.cpp @@ -126,7 +126,7 @@ std::optional GetExecutablePath() { } #elif defined(DAWN_PLATFORM_FUCHSIA) std::optional GetExecutablePath() { - // TODO: Implement on Fuchsia + UNIMPLEMENTED(); return {}; } #elif defined(DAWN_PLATFORM_EMSCRIPTEN) diff --git a/src/dawn/native/d3d12/RenderPassBuilderD3D12.cpp b/src/dawn/native/d3d12/RenderPassBuilderD3D12.cpp index 0c7a9c817c..9e72e758e9 100644 --- a/src/dawn/native/d3d12/RenderPassBuilderD3D12.cpp +++ b/src/dawn/native/d3d12/RenderPassBuilderD3D12.cpp @@ -69,19 +69,9 @@ namespace dawn::native::d3d12 { } // RESOLVE_MODE_AVERAGE is only valid for non-integer formats. - // TODO: Investigate and determine how integer format resolves should work in WebGPU. - switch (resolveDestination->GetFormat().GetAspectInfo(Aspect::Color).baseType) { - case wgpu::TextureComponentType::Sint: - case wgpu::TextureComponentType::Uint: - resolveParameters.ResolveMode = D3D12_RESOLVE_MODE_MAX; - break; - case wgpu::TextureComponentType::Float: - resolveParameters.ResolveMode = D3D12_RESOLVE_MODE_AVERAGE; - break; - - case wgpu::TextureComponentType::DepthComparison: - UNREACHABLE(); - } + ASSERT(resolveDestination->GetFormat().GetAspectInfo(Aspect::Color).baseType == + wgpu::TextureComponentType::Float); + resolveParameters.ResolveMode = D3D12_RESOLVE_MODE_AVERAGE; resolveParameters.SubresourceCount = 1; diff --git a/src/dawn/native/metal/RenderPipelineMTL.mm b/src/dawn/native/metal/RenderPipelineMTL.mm index 18adb692de..4c0f4b1689 100644 --- a/src/dawn/native/metal/RenderPipelineMTL.mm +++ b/src/dawn/native/metal/RenderPipelineMTL.mm @@ -327,8 +327,8 @@ namespace dawn::native::metal { AcquireNSRef([MTLRenderPipelineDescriptor new]); MTLRenderPipelineDescriptor* descriptorMTL = descriptorMTLRef.Get(); - // TODO: MakeVertexDesc should be const in the future, so we don't need to call it here when - // vertex pulling is enabled + // TODO(dawn:1384): MakeVertexDesc should be const in the future, so we don't need to call + // it here when vertex pulling is enabled NSRef vertexDesc = MakeVertexDesc(); // Calling MakeVertexDesc first is important since it sets indices for packed bindings diff --git a/src/dawn/native/metal/TextureMTL.mm b/src/dawn/native/metal/TextureMTL.mm index dc04324a9b..7f0e13c25b 100644 --- a/src/dawn/native/metal/TextureMTL.mm +++ b/src/dawn/native/metal/TextureMTL.mm @@ -317,7 +317,7 @@ namespace dawn::native::metal { if (@available(macOS 10.12, iOS 13.0, *)) { return MTLPixelFormatDepth16Unorm; } else { - // TODO (dawn:1181): Allow non-conformant implementation on macOS 10.11 + // TODO(dawn:1181): Allow non-conformant implementation on macOS 10.11 UNREACHABLE(); } case wgpu::TextureFormat::Stencil8: @@ -654,8 +654,6 @@ namespace dawn::native::metal { // Metal only allows format reinterpretation to happen on swizzle pattern or conversion // between linear space and sRGB. For example, creating bgra8Unorm texture view on // rgba8Unorm texture or creating rgba8Unorm_srgb texture view on rgab8Unorm texture. - // TODO: add MTLTextureUsagePixelFormatView when needed when we support other format - // reinterpretation. mtlDesc.usage = MetalTextureUsage(GetFormat(), GetInternalUsage(), GetSampleCount()); mtlDesc.pixelFormat = MetalPixelFormat(GetFormat().format); mtlDesc.mipmapLevelCount = GetNumMipLevels(); diff --git a/src/dawn/native/vulkan/CommandBufferVk.cpp b/src/dawn/native/vulkan/CommandBufferVk.cpp index 97a9a0d51d..6408f29415 100644 --- a/src/dawn/native/vulkan/CommandBufferVk.cpp +++ b/src/dawn/native/vulkan/CommandBufferVk.cpp @@ -668,7 +668,6 @@ namespace dawn::native::vulkan { copy->copySize.depthOrArrayLayers)); } - // TODO after Yunchao's CL ToBackend(src.texture) ->TransitionUsageNow(recordingContext, wgpu::TextureUsage::CopySrc, srcRange); diff --git a/src/dawn/native/vulkan/SwapChainVk.cpp b/src/dawn/native/vulkan/SwapChainVk.cpp index f2e06796b6..03daa6d503 100644 --- a/src/dawn/native/vulkan/SwapChainVk.cpp +++ b/src/dawn/native/vulkan/SwapChainVk.cpp @@ -309,7 +309,7 @@ namespace dawn::native::vulkan { DAWN_TRY_ASSIGN(mConfig, ChooseConfig(surfaceInfo)); - // TODO Choose config instead of hardcoding + // TODO(dawn:269): Choose config instead of hardcoding VkSwapchainCreateInfoKHR createInfo; createInfo.sType = VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR; createInfo.pNext = nullptr; @@ -514,7 +514,8 @@ namespace dawn::native::vulkan { CommandRecordingContext* recordingContext = device->GetPendingRecordingContext(); if (mConfig.needsBlit) { - // TODO ditto same as present below: eagerly transition the blit texture to CopySrc. + // TODO(dawn:269): ditto same as present below: eagerly transition the blit texture to + // CopySrc. mBlitTexture->TransitionUsageNow(recordingContext, wgpu::TextureUsage::CopySrc, mBlitTexture->GetAllSubresources()); mTexture->TransitionUsageNow(recordingContext, wgpu::TextureUsage::CopyDst, diff --git a/src/dawn/node/binding/GPU.cpp b/src/dawn/node/binding/GPU.cpp index 903ae136a0..55be4160ed 100644 --- a/src/dawn/node/binding/GPU.cpp +++ b/src/dawn/node/binding/GPU.cpp @@ -60,7 +60,7 @@ namespace wgpu::binding { // wgpu::bindings::GPU //////////////////////////////////////////////////////////////////////////////// GPU::GPU(Flags flags) : flags_(std::move(flags)) { - // TODO: Disable in 'release' + // TODO(dawn:1123): Disable in 'release' instance_.EnableBackendValidation(true); instance_.SetBackendValidationLevel(dawn::native::BackendValidationLevel::Full); diff --git a/src/dawn/node/binding/GPUBuffer.cpp b/src/dawn/node/binding/GPUBuffer.cpp index 64bc7d0965..2fd330c1b8 100644 --- a/src/dawn/node/binding/GPUBuffer.cpp +++ b/src/dawn/node/binding/GPUBuffer.cpp @@ -97,8 +97,8 @@ namespace wgpu::binding { break; case WGPUBufferMapAsyncStatus_Unknown: case WGPUBufferMapAsyncStatus_DeviceLost: - // TODO: The spec is a bit vague around what the promise should do - // here. + // TODO(dawn:1123): The spec is a bit vague around what the promise should + // do here. c->promise.Reject(Errors::UnknownError(c->env)); break; } diff --git a/src/dawn/samples/ManualSwapChainTest.cpp b/src/dawn/samples/ManualSwapChainTest.cpp index 1de126b7fa..397842d8a6 100644 --- a/src/dawn/samples/ManualSwapChainTest.cpp +++ b/src/dawn/samples/ManualSwapChainTest.cpp @@ -267,7 +267,7 @@ int main(int argc, const char* argv[]) { } // Choose an adapter we like. - // TODO: allow switching the window between devices. + // TODO(dawn:269): allow switching the window between devices. DawnProcTable procs = dawn::native::GetProcs(); dawnProcSetProcs(&procs); diff --git a/src/dawn/tests/end2end/DeviceLostTests.cpp b/src/dawn/tests/end2end/DeviceLostTests.cpp index 188f8ddc98..853807d403 100644 --- a/src/dawn/tests/end2end/DeviceLostTests.cpp +++ b/src/dawn/tests/end2end/DeviceLostTests.cpp @@ -391,8 +391,8 @@ TEST_P(DeviceLostTest, GetMappedRange_MapAsyncWriting) { ASSERT_EQ(buffer.GetConstMappedRange(), rangeBeforeLoss); } -// TODO mapasync read + resolve + loss getmappedrange != nullptr. -// TODO mapasync write + resolve + loss getmappedrange != nullptr. +// TODO(dawn:929): mapasync read + resolve + loss getmappedrange != nullptr. +// TODO(dawn:929): mapasync write + resolve + loss getmappedrange != nullptr. // Test that Command Encoder Finish fails when device lost TEST_P(DeviceLostTest, CommandEncoderFinishFails) { diff --git a/src/dawn/tests/end2end/SamplerTests.cpp b/src/dawn/tests/end2end/SamplerTests.cpp index b675c79409..c2c983045f 100644 --- a/src/dawn/tests/end2end/SamplerTests.cpp +++ b/src/dawn/tests/end2end/SamplerTests.cpp @@ -158,7 +158,6 @@ class SamplerTest : public DawnTest { EXPECT_PIXEL_RGBA8_EQ(expectedU3, mRenderPass.color, 3, 0); EXPECT_PIXEL_RGBA8_EQ(expectedV2, mRenderPass.color, 0, 2); EXPECT_PIXEL_RGBA8_EQ(expectedV3, mRenderPass.color, 0, 3); - // TODO: add tests for W address mode, once Dawn supports 3D textures } utils::BasicRenderPass mRenderPass; diff --git a/src/dawn/tests/end2end/TextureSubresourceTests.cpp b/src/dawn/tests/end2end/TextureSubresourceTests.cpp index caa94c722d..391d94434d 100644 --- a/src/dawn/tests/end2end/TextureSubresourceTests.cpp +++ b/src/dawn/tests/end2end/TextureSubresourceTests.cpp @@ -188,14 +188,6 @@ TEST_P(TextureSubresourceTest, ArrayLayersTest) { EXPECT_TEXTURE_EQ(&bottomLeft, texture, {0, kSize - 1, 1}, {1, 1}); } -// TODO (yunchao.he@intel.com): -// * add tests for storage texture and sampler across miplevel or -// arraylayer dimensions in the same texture -// -// * add tests for copy operation upon texture subresource if needed -// -// * add tests for clear operation upon texture subresource if needed - DAWN_INSTANTIATE_TEST(TextureSubresourceTest, D3D12Backend(), MetalBackend(), diff --git a/src/dawn/tests/end2end/VertexStateTests.cpp b/src/dawn/tests/end2end/VertexStateTests.cpp index e92b6f07c5..2089cd6250 100644 --- a/src/dawn/tests/end2end/VertexStateTests.cpp +++ b/src/dawn/tests/end2end/VertexStateTests.cpp @@ -648,14 +648,6 @@ DAWN_INSTANTIATE_TEST(VertexStateTest, OpenGLESBackend(), VulkanBackend()); -// TODO for the input state: -// - Add more vertex formats -// - Add checks that the stride is enough to contain all attributes -// - Add checks stride less than some limit -// - Add checks for alignement of vertex buffers and attributes if needed -// - Check for attribute narrowing -// - Check that the input state and the pipeline vertex input types match - class OptionalVertexStateTest : public DawnTest {}; // Test that vertex input is not required in render pipeline descriptor. diff --git a/src/dawn/tests/unittests/validation/ResourceUsageTrackingTests.cpp b/src/dawn/tests/unittests/validation/ResourceUsageTrackingTests.cpp index df148a4536..bbc85f7683 100644 --- a/src/dawn/tests/unittests/validation/ResourceUsageTrackingTests.cpp +++ b/src/dawn/tests/unittests/validation/ResourceUsageTrackingTests.cpp @@ -1690,11 +1690,4 @@ namespace { } } - // TODO (yunchao.he@intel.com): - // - // * Add tests for multiple encoders upon the same resource simultaneously. This situation fits - // some cases like VR, multi-threading, etc. - // - // * Add tests for bundle - } // anonymous namespace diff --git a/src/dawn/tests/unittests/validation/TextureSubresourceTests.cpp b/src/dawn/tests/unittests/validation/TextureSubresourceTests.cpp index 583486315b..4986d98383 100644 --- a/src/dawn/tests/unittests/validation/TextureSubresourceTests.cpp +++ b/src/dawn/tests/unittests/validation/TextureSubresourceTests.cpp @@ -133,12 +133,4 @@ namespace { TestRenderPass(samplerView, renderView); } - // TODO (yunchao.he@intel.com): - // * Add tests for compute, in which texture subresource is traced per dispatch. - // - // * Add tests for multiple encoders upon the same resource simultaneously. This situation fits - // some cases like VR, multi-threading, etc. - // - // * Add tests for conflicts between usages in two render bundles used in the same pass. - } // anonymous namespace