WeakPtrFactory should be destroyed before any other members.

[0xZOne]

WeakPtrFactory should remain the last member so it'll be destroyed and invalidate its weak pointers before any other members are destroyed.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
  writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

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.

[dnfield]

We should not move ivars below methods/friend declarations for testing. That goes against the style guide.

There are two "real" instances here where it could matter: accessibility_bridge.h and rasterizer.h. We're lucky in that neither of those declare an ivar that actually uses the weak pointer that the factory would vend.

Given that, we should definitely take whatever time needed to add analysis/tests for this.

[0xZOne]

We should not move ivars below methods/friend declarations for testing. That goes against the style guide.

There are two "real" instances here where it could matter: accessibility_bridge.h and rasterizer.h. We're lucky in that neither of those declare an ivar that actually uses the weak pointer that the factory would vend.

Given that, we should definitely take whatever time needed to add analysis/tests for this.

Thank you for your reply.

As per Declaration Order, all data members should be last. Another purpose here is to alleviate the risk of new members being mistakenly inserted after |WeakPtrFactory|.

[dnfield]

So I learned! But if we're moving them we should move all of them.

I also see that there are some objc files that are impacted by this but not fixed.

[0xZOne]

So I learned! But if we're moving them we should move all of them.

I also see that there are some objc files that are impacted by this but not fixed.

Please have another look at it, Thanks.

[dnfield]

This still needs some kind of test (static analysis would be fine) to make sure that these rules are followed.

[0xZOne]

This still needs some kind of test (static analysis would be fine) to make sure that these rules are followed.

Got it. Are there any examples I can refer to?

[dnfield]

I'm not sure. We do run clang-tidy but I don't think that can detect this.

[0xZOne]

I'm not sure. We do run clang-tidy but I don't think that can detect this.

OK, thanks anyway. :)

[0xZOne]

@dnfield

I found that chromium has a clang plugin to check the WeakPtrFactory class member order. Maybe we can port the clang plugin to flutter.

See FindBadConstructsConsumer::CheckWeakPtrFactoryMembers for details.

I want to finish it. However, I am not familiar with the build system, nor do I have time to dig into it recently. Any bits of advice? Thanks.

[dnfield]

Talked to some Fuchsia Toolchain folks, and they suggested implementing these as clang-tidy checks. Fuchsia already has some at https://github.com/llvm/llvm-project/tree/main/clang-tools-extra/clang-tidy/fuchsia

I am not clear on who might be available to do this, if anyone is right now, or if this is something @0xZOne would be interestedin trying, but just trying to leave some more breadcrumbs here.

[dnfield]

This LGTM but will need a test exemption from @Hixie to land.

[dnfield]

Also, per our updated policy, this needs a review from someone else with commit access to the repo before landing.

[Hixie]

test-exempt: only affects analysis

Please document the desired analysis in an issue and link that issue here. Thanks.

[0xZOne]

test-exempt: only affects analysis

Please document the desired analysis in an issue and link that issue here. Thanks.

Related issue: #93039