More KJ_IF_SOME changes to workerd/api/streams #1215
Conversation
ohodson
changed the title
This change leaves just standard.c++ to do. That has dangling-else issues when migrated to KJ_IF_SOME. Bug: EW-7618 Test: bazel test //...
Bug: EW-7618 Test: bazel test //...
ohodson
force-pushed
the
orion/kj_if_some_streams
branch
from
ed11a55
to
9506197
Compare
|
Internal PR is 6269, passes tests there. |
|
Internal PR is 6271, passes tests there (just waiting on x64-asan-release, passes x64-asan). |
| KJ_IF_SOME(pendingCancel, maybePendingCancel) { | ||
| maybeResolvePromise(js, pendingCancel.fulfiller); | ||
| } else { | ||
| // Else block to avert dangling else compiler warning. |
There was a problem hiding this comment.
huh, what is the compiler warning about when the else isn't here? Does KJ_IF_SOME generate an empty else?
There was a problem hiding this comment.
KJ_IF_SOME has dangling if suppressions for the compiler, but it still complains in a few places. Complaints typically come with nested KJ_IF_SOME blocks, but also occur with a few other constructs.
There was a problem hiding this comment.
Can we fix KJ_IF_SOME for those cases?
There was a problem hiding this comment.
I don't think we have a fix for now, or at least one we like, @harrishancock might have thoughts on this. I've come across maybe 7-8 places that it is an issue. For now, it seems reasonable to include the small intervention and document it, so it is a quick clean-up in future.
The problem is caused by KJ_IF_SOME wanting to support KJ_IF_SOME(..) {...} else {...}, when two are nested there are two if statements without else blocks and that is where the ambiguity comes from.
We might be able replace an if for a for, but that might also have drawbacks. Not tried.
There was a problem hiding this comment.
We might be able replace an
iffor afor, but that might also have drawbacks. Not tried.
Ignore this, I don't see a way to make that work and support nesting.
Thanks for reviewing. There are two commits that can be considered separately in the UX; still a good size though. The second commit is most of the work here and it's just |
|
Ahh, I don't tend to review commit-by-commit. |
Finishes off workerd/api/streams