Repository navigation
[engine] Replace fml::Status and fml::StatusOr with absl - #194161
Conversation
|
Let's replace usages of fml::Status with absl::Status instead. |
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
gaaclarke
left a comment
There was a problem hiding this comment.
oops meant to "request changes", not close the issue. we want absl::Status
fml::Status stored the message as a std::string_view, so a status built from a runtime string pointed into that string's buffer. CommandQueueVK returns such a status from a lambda whose local string is gone by the time the caller reads message(). The message is now a std::string; message() still returns a std::string_view, so callers do not change.
Review asked to use absl::Status instead of fixing fml::Status. absl::Status owns its message, so the dangling view in CommandQueueVK::Submit goes away with the type. fml/status.h, fml/status_or.h and the status test are removed. absl::Status is [[nodiscard]], so call sites that dropped a status now say so with IgnoreError(), or check it in tests. CopyImageByteData built an error for a failed swizzle surface without returning it and then used the null surface; it now returns the error.
6591a96 to
c75037a
Compare
|
@gaaclarke Done: |
…s-owns-message # Conflicts: # engine/src/flutter/lib/gpu/command_buffer_unittests.cc
gaaclarke
left a comment
There was a problem hiding this comment.
lgtm, i was sort of waiting for this to be lifted out of draft =T
There was a problem hiding this comment.
Code Review
This pull request replaces the custom fml::Status and fml::StatusOr classes with absl::Status and absl::StatusOr from the Abseil library across the repository, resulting in the deletion of the FML status headers. The review feedback highlights two instances in descriptor_pool_vk.cc where the status returned by CreateNewPool is ignored, which could lead to undefined behavior or crashes if pool allocation fails.
Master replaced fml::Status with absl (flutter#194161).
) This started as a fix for `fml::Status` holding its message as a `std::string_view`: `CommandQueueVK::Submit` built a status from a local string inside the fence callback and returned it after the string was gone. In review the ask was to use `absl::Status` instead, so this PR removes `fml::Status` and `fml::StatusOr` and switches every user to `absl::Status` and `absl::StatusOr`. `absl::Status` owns its message, so the dangling view goes away with the type, and so does the TODO in `status_or.h` that pointed at flutter#134741. Most of it is mechanical. `fml::Status()` becomes `absl::OkStatus()`, the status codes have the same names in both, includes point at `third_party/abseil-cpp/absl/status`, and `//flutter/flow` gets abseil status in `public_deps` because `frame_timings.h` returns it. The part that isn't mechanical comes from `absl::Status` being `[[nodiscard]]`. `fml::Status` wasn't, and about 25 call sites dropped a status without looking at it. I didn't want this PR to change what any of them do, so in production code and in the render and playground tests they now call `.IgnoreError()`. That includes `DescriptorPoolVK::AllocateDescriptorSets`, which ignores a failed `CreateNewPool` today. Whether it should return that error is a separate question. The fence waiter, descriptor pool and command encoder tests check the result instead, since there the expected status is known. One exception. `CopyImageByteData` in `lib/ui/painting/image_encoding.cc` built an error for a failed swizzle surface, never returned it, and went on to use the null surface. The compiler only caught it because of `[[nodiscard]]`, and I added the missing `return`. If you'd rather have that as its own PR I'll split it out. **Where it was verified.** macOS arm64, `host_debug_unopt_arm64`, rebased on current main, `gn check` clean. `fml_unittests` 203/203, `ui_unittests` 73/73, `shell_unittests` 197/197, `flow_unittests` 313/313 and the Vulkan, fence waiter, descriptor pool, command queue, Gaussian blur and text frame tests in `impeller_unittests` (193/193). `impeller_golden_tests` and `flutter_tester` build. Locally `PerformanceOverlayLayerDefault.Gold` and two `EmbedderTest.CanRenderTextWithImpeller*Metal` tests fail on missing glyphs and ICU data (`U_MISSING_RESOURCE_ERROR`). They fail the same way on main without this change, so it's my checkout. CI covers the other platforms. Fixes flutter#194151 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This started as a fix for
fml::Statusholding its message as astd::string_view:CommandQueueVK::Submitbuilt a status from a local string inside the fence callback and returned it after the string was gone. In review the ask was to useabsl::Statusinstead, so this PR removesfml::Statusandfml::StatusOrand switches every user toabsl::Statusandabsl::StatusOr.absl::Statusowns its message, so the dangling view goes away with the type, and so does the TODO instatus_or.hthat pointed at #134741.Most of it is mechanical.
fml::Status()becomesabsl::OkStatus(), the status codes have the same names in both, includes point atthird_party/abseil-cpp/absl/status, and//flutter/flowgets abseil status inpublic_depsbecauseframe_timings.hreturns it.The part that isn't mechanical comes from
absl::Statusbeing[[nodiscard]].fml::Statuswasn't, and about 25 call sites dropped a status without looking at it. I didn't want this PR to change what any of them do, so in production code and in the render and playground tests they now call.IgnoreError(). That includesDescriptorPoolVK::AllocateDescriptorSets, which ignores a failedCreateNewPooltoday. Whether it should return that error is a separate question. The fence waiter, descriptor pool and command encoder tests check the result instead, since there the expected status is known.One exception.
CopyImageByteDatainlib/ui/painting/image_encoding.ccbuilt an error for a failed swizzle surface, never returned it, and went on to use the null surface. The compiler only caught it because of[[nodiscard]], and I added the missingreturn. If you'd rather have that as its own PR I'll split it out.Where it was verified. macOS arm64,
host_debug_unopt_arm64, rebased on current main,gn checkclean.fml_unittests203/203,ui_unittests73/73,shell_unittests197/197,flow_unittests313/313 and the Vulkan, fence waiter, descriptor pool, command queue, Gaussian blur and text frame tests inimpeller_unittests(193/193).impeller_golden_testsandflutter_testerbuild. LocallyPerformanceOverlayLayerDefault.Goldand twoEmbedderTest.CanRenderTextWithImpeller*Metaltests fail on missing glyphs and ICU data (U_MISSING_RESOURCE_ERROR). They fail the same way on main without this change, so it's my checkout. CI covers the other platforms.Fixes #194151
Pre-launch Checklist
///).