[go: nahoru, domu]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[lint] Merge impeller .clang-tidy into main config #33692

Merged
merged 14 commits into from
Jun 21, 2022
Merged

[lint] Merge impeller .clang-tidy into main config #33692

merged 14 commits into from
Jun 21, 2022

Conversation

cbracken
Copy link
Member
@cbracken cbracken commented May 30, 2022 •
edited

Merges most (but not all) of the impeller .clang-tidy rules into the
main .clang-tidy config. Merges:

  • readability-identifier-naming.PrivateMemberSuffix (_)
  • readability-identifier-naming.EnumConstantPrefix (k)
  • modernize-use-default-member-init.UseAssignment

Does not merge:

  • readability-identifier-naming.PublicMethodCase (CamelCase)
  • readability-identifier-naming.PrivateMethodCase (CamelCase)

These last two are not merged due to the non-trivial number of existing
field accessors that use field_name() methods to directly return
field_name_. While these are permitted by the C++ style guide, we may
want to move to a single, simple rule and name everything in CamelCase.
These can be enabled in a followup patch.

No new tests added, since this change is style-only.

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.

@cbracken
Copy link
Member Author

This change is organised into a stack of commits, one per option, for ease of review.

@cbracken cbracken force-pushed the clang-tidy-merge-partial branch 4 times, most recently from 174fb78 to c03b557 Compare May 30, 2022 21:40
@cbracken
Copy link
Member Author

@chinmaygarde added three more commits to fix up the last of the issues raised in the presubmit. The first two are non-controversial. I've added some notes to the last commit description; whether we delete the dead store or ignore it is really up to the author's intent with leaving it there.

@cbracken
Copy link
Member Author
cbracken commented May 30, 2022 •
edited

It's interesting that it seems to be upset about the local result from Parser::parse getting captured by the local parser in Environment::parse during a chained return. It looks like the only thing captured by the template is a copy of the input string.

I could tack in a // NOLINTNEXTLINE(clang-analyzer-core.StackAddressEscape) above our call to Environment::parse but to be honest I haven't yet completely convinced myself there's no issue here.

The stack of calls looks something like:

@cbracken
Copy link
Member Author
cbracken commented Jun 1, 2022

Hrm. Can't find an easy way to work around the clang-analyzer-core.StackAddressEscape issue since it's in a header library. I did try jamming a NOLINTBEGIN/NOLINTEND pair around the #include as a desperate measure, but didn't work :/

Any thoughts?

@zanderso
Copy link
Member
zanderso commented Jun 2, 2022

@chinmaygarde wants to take a look at the underlying issue in inja.

@cbracken
Copy link
Member Author
cbracken commented Jun 2, 2022 •
edited

SGTM! On the surface it looks sane but there's also enough parameters passed to locals, then returned that, as I say, I haven't yet completely convinced myself there's no issue there.

@chinmaygarde chinmaygarde mentioned this pull request Jun 9, 2022
Closed
@chinmaygarde
Copy link
Member

I haven't had the chance to take a look at the Inja issue. Frankly, this escaped my attention. Filed flutter/flutter#105732 and assigned to myself. Marking this WIP in the meantime. Will continue with this after the linked issue is fixed.

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Jun 9, 2022
@chinmaygarde
Copy link
Member

@cbracken I won't be able to get to this before the end of the month realistically. Are you blocked on this? Since there is an issue filed for it already, maybe you can proceed with the NOLINT?

@cbracken
Copy link
Member Author

Not blocked on this; but I'll add the nolint to land it before I forget. Thanks.

@skia-gold
Copy link

Gold has detected about 2 new digest(s) on patchset 14.
View them at https://flutter-engine-gold.skia.org/cl/github/33692

@cbracken
Copy link
Member Author
cbracken commented Jun 21, 2022 •
edited

@chinmaygarde to re-check 2e33201 which is a non-stylistic change, but rather prevents use-after-move warnings int the test expectations below.

Broke out into #34197

@cbracken
Eliminate use-after-move
909833c 
pipeline_desc is a std::optional<PipelineDescriptor>. It's used in test
expectations after (lines 320-321) the move. This passes by value like
in other tests.
@cbracken
[lint] Merge impeller .clang-tidy into main config
f106172 
Merges most (but not all) of the impeller .clang-tidy rules into the
main .clang-tidy config. Merges:
* readability-identifier-naming.PrivateMemberSuffix (_)
* readability-identifier-naming.EnumConstantPrefix (k)
* modernize-use-default-member-init.UseAssignment

Does not merge:
* readability-identifier-naming.PublicMethodCase (CamelCase)
* readability-identifier-naming.PrivateMethodCase (CamelCase)

These last two are not merged due to the non-trivial number of existing
field accessors that use `field_name()` methods to directly return
`field_name_`. While these are permitted by the C++ style guide, we may
want to move to a single, simple rule and name everything in CamelCase.

These can be enabled in a followup patch.
@cbracken
Private fields end in _
c33aab0
@cbracken cbracken mentioned this pull request Jun 21, 2022
Merged
8 tasks
@cbracken
Add missing enum k prefixes
cdacee9
@cbracken
Enable modernize-use-default-member-init
88b5a99
@cbracken
Re-order lint config alphabetically
c398877
@cbracken
Make TestPassDelegate ctor explicit
e8d7a20
@cbracken
Ignore clang-analyzer-deadcode.DeadStores in tests
487d428 
Adds NOLINT annotation to a couple of dead stores used to verify
locking.
@cbracken
Ignore clang-analyzer-deadcode.DeadStores in BlobLibrary ctor
1c801c9 
This is a dead store which we may want to eliminate. We do read from
this field earlier in the constructor, and the offset should
conceptually be incremented here, but it's never (currently) read after
the conditional + memcpy earlier in the method. I'm assuming it may have
been kept intentionally as potential future-proofing in case of future
use of offset later in the method. This write can safely be removed with
the current code though.
@cbracken
Update two enums to kCamelCase style
a9f40b1
@cbracken
Add NOLINT for inja dependency
1df6c14
@cbracken
Don't warn on Metal const names
c59239b
@cbracken
Use kKeyboardModifier enum values
28894a9
@cbracken
iOS text input enums prefixed with k
726ab40
@cbracken cbracken merged commit 31afa38 into flutter:main Jun 21, 2022
@cbracken cbracken deleted the clang-tidy-merge-partial branch June 21, 2022 18:52
@cbracken cbracken removed the Work in progress (WIP) Not ready (yet) for review! label Jun 21, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 21, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 21, 2022
@engine-flutter-autoroll
31afa38 [lint] Merge impeller .clang-tidy into main config (flutter/e…
18d3aa0 
…ngine#33692)
This was referenced Jun 27, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

Assignees

@cbracken cbracken

Labels
affects: engine e: impeller platform-macos
Projects
No open projects
Impeller
Archived in project
Milestone
No milestone
4 participants
@cbracken @zanderso @chinmaygarde @skia-gold