Initialize members and check for nullptr #30941
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
The comments about default initializing the values in the header are nits. But bumping the submitted frame count when there is no frame must be addressed however.
| GrDirectContext* context, | ||
| std::unique_ptr<SurfaceFrame> frame) { | ||
| frame->Submit(); | ||
| if (frame) { |
There was a problem hiding this comment.
If the frame pointer is null (checked later), there must be an early return so the submitted frame count in not bumped up.
So something like.
if (!frame) {
return;
}
followed by the removal of frame && later on like 70 since that check will now be redundant.
| : TaskRunner(nullptr /* loop implemenation*/), | ||
| embedder_identifier_(embedder_identifier), | ||
| dispatch_table_(std::move(table)), | ||
| last_baton_(0), |
There was a problem hiding this comment.
Good call. But, consider defaulting it in the header in case another ctor is added later. Something like uint64_t last_baton_ = 0; in embedder_task_runner.h.
| messenger, | ||
| kChannelName, | ||
| &flutter::JsonMethodCodec::GetInstance())), | ||
| client_id_(0), |
There was a problem hiding this comment.
Nit but ditto about defaulting in the header.
chinmaygarde
added
the
waiting for tree to go green
|
This pull request is not suitable for automatic merging in its current state.
|
fluttergithubbot
removed
the
waiting for tree to go green
|
Unfortunately, this needs to be rebased on ToT for the presubmits to pass. Can you please do the same so I may add the CQ tag? |
There is a check on line 70 if frame is nullptr so probably it should be also checked before frame->Submit().
m-bialek
force-pushed
the
upstream_fixes
branch
from
b0ced21 to
d2ac075
Compare
|
@chinmaygarde Done |
chinmaygarde
added
the
waiting for tree to go green
4 participants
Hi! Static analysis pointed out a few places with potential errors so maybe you will be interested in changes I did:
frameisnullptrbeforeframe->Submit();(there is a similar check on the next lines)last_baton_(it is used but never initialized)client_id_(it is set in another method but I believe it should be initialized)Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.