Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[video_player_platform_interface] Migrate to null safety #3159

Merged
merged 4 commits into from Oct 16, 2020

Conversation

blasten
Copy link

@blasten blasten commented Oct 15, 2020

Null safety migration.

FYI @gaaclarke the Dart code generated by Pigeon had trailing whitespaces.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read through the pigeon stuff, looks good to me. The addition of the dart-null-saftey flag happens in a different repo right?

@blasten
Copy link
Author

blasten commented Oct 15, 2020

The linter for darwin plugins is complaining about the generated file messages.m. https://api.cirrus-ci.com/v1/task/4714658705702912/logs/main.log

FYI @gaaclarke

@jmagman do you know if there's a way to disable the linter for auto generated files?

@jmagman
Copy link
Member

jmagman commented Oct 15, 2020

The linter for darwin plugins is complaining about the generated file messages.m. https://api.cirrus-ci.com/v1/task/4714658705702912/logs/main.log

FYI @gaaclarke

@jmagman do you know if there's a way to disable the linter for auto generated files?

I don't think we should ignore it, our developers using the plugin will see the same warning. Can we fix the generation?

@gaaclarke
Copy link
Member

flutter/flutter#63966 This is the issue. If i remember correctly that it is no longer an analysis error in later versions of clang =(

@gaaclarke
Copy link
Member

This analyzer can be turned off with the CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION environment variable. Since the objection isn't a problem according to later versions of clang I'd rather disable that check. You might be able to do it in the source file too but I didn't see a way.

@gaaclarke
Copy link
Member

Alternatively, you can use the __clang_analyzer__ macro if you don't want to affect the other code. Maybe we should add a flag for that since everyone could have all different analyzer settings.

@blasten
Copy link
Author

blasten commented Oct 16, 2020

@gaaclarke Done.

@jmagman I agree. Ideally, the analyzer doesn't complain, but mostly likely we won't be able to enforce the same analyzer rules/version.

@blasten blasten merged commit ce3b264 into flutter:nnbd Oct 16, 2020
@blasten blasten deleted the nnbd_video_player_platform_interface branch October 16, 2020 01:36
@jmagman
Copy link
Member

jmagman commented Oct 16, 2020

@jmagman I agree. Ideally, the analyzer doesn't complain, but mostly likely we won't be able to enforce the same analyzer rules/version.

True we can't enforce it, but most people won't change the default from the flutter create template. Ideally the plugins would have as many checks on as possible and then not have warnings in the strictest mode.

@blasten
Copy link
Author

blasten commented Oct 16, 2020

Agreed. For my own understanding, what file is defining the linter rules? Is this file/configuration from flutter create or custom made for the plugins repo?

@jmagman
Copy link
Member

jmagman commented Oct 16, 2020

@gaaclarke
Copy link
Member

many checks on as possible and then not have warnings in the strictest mode.

The problem is that clang has redefined the meaning of some checks. In this case something that was a warning in one version became acceptable in a later version. It's untenable to maintain support for the union of the strictest settings of all versions of clang for all possible settings. Ideally we run the analyzer once as part of CI for pigeon, then opt out of analysis when it's used (with a flag to turn it on if people want).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants