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.

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

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

Merged
merged 14 commits into from
Jun 21, 2022

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented May 30, 2022

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

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

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
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

@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

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.
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 cbracken mentioned this pull request Jun 21, 2022
8 tasks
Adds NOLINT annotation to a couple of dead stores used to verify
locking.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants