-
Notifications
You must be signed in to change notification settings - Fork 6k
Eliminate duplicated code when dealing with pointer data #36822
Conversation
memcpy(&data_[i * sizeof(PointerData)], &data, sizeof(PointerData)); | ||
} | ||
|
||
PointerData PointerDataPacket::GetPointerData(size_t i) const { | ||
FML_DCHECK(i < GetLength()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DCHECK only runs in debug mode. if the goal of this PR is to add bounds checking to make it safe, you need to always perform the bounds check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added DCHECK because originally there is no check at all and I dare not change semantics of the code (e.g. if no check because previous dev deliberately do so to avoid being too slow?). If you like, I will change to CHECK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM as long as the goal is just to reduce some code duplication. I'm going to update the title to remove the safety parts since it's not actually any safer than the old code.
memcpy
and duplicated code when dealing with pointer data
auto label is removed for flutter/engine, pr: 36822, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead. |
auto label is removed for flutter/engine, pr: 36822, due to Validations Fail. |
@dnfield Sure. Btw if you guys think the cost paid by CHECK is ok, I will change DCHECK to CHECK (pretty easy to change) |
CHECK will cause a crash at runtime. DCHECK is probably fine here to make tests fail. |
…115118) * 4f86f229d Update FlutterView.java (flutter/engine#37312) * 80e31edb2 Adding release_build:true to Mac builds. (flutter/engine#37478) * 2f5b7ac34 Eliminate duplicated code when dealing with pointer data (flutter/engine#36822) * f831382b7 Install CIPD ninja using DEPS (flutter/engine#37375) * 4cb9c1e00 mouse-input-view nit and add mouse-input-test to integration test script (flutter/engine#37441)
* add methods * add empty test * add unit test * Update licenses_flutter * add const * size -> get length * add boundary checks * add tests * remove death test since it says "Death tests use fork(), which is unsafe particularly in a threaded context."
…lutter#115118) * 4f86f229d Update FlutterView.java (flutter/engine#37312) * 80e31edb2 Adding release_build:true to Mac builds. (flutter/engine#37478) * 2f5b7ac34 Eliminate duplicated code when dealing with pointer data (flutter/engine#36822) * f831382b7 Install CIPD ninja using DEPS (flutter/engine#37375) * 4cb9c1e00 mouse-input-view nit and add mouse-input-test to integration test script (flutter/engine#37441)
…lutter#115118) * 4f86f229d Update FlutterView.java (flutter/engine#37312) * 80e31edb2 Adding release_build:true to Mac builds. (flutter/engine#37478) * 2f5b7ac34 Eliminate duplicated code when dealing with pointer data (flutter/engine#36822) * f831382b7 Install CIPD ninja using DEPS (flutter/engine#37375) * 4cb9c1e00 mouse-input-view nit and add mouse-input-test to integration test script (flutter/engine#37441)
Hope the PR is self-explanatory :) If needed I can explain it.
List which issues are fixed by this PR. You must list at least one issue.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.