Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Clang-tidy: cleaned up the output.#37059

Merged
auto-submit[bot] merged 3 commits intoflutter:mainfrom
gaaclarke:clang-tidy-cleaner-output
Nov 2, 2022
Merged

Clang-tidy: cleaned up the output.#37059
auto-submit[bot] merged 3 commits intoflutter:mainfrom
gaaclarke:clang-tidy-cleaner-output

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Oct 26, 2022

Previously we would print out errors twice (or thrice if you had --verbose). Also we were printing out the invocation which isn't necessary now that the --verbose flag does that. Users want to know the filename and a quick description of the errors.

issue: flutter/flutter#113848

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

flutter-dashboard bot commented Oct 26, 2022

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

@gaaclarke gaaclarke requested review from jmagman and zanderso October 26, 2022 22:25
@gaaclarke
Copy link
Member Author

gaaclarke commented Oct 26, 2022

Here is an example output for one file with the current state:

❌ Failures for clang-tidy on /Users/aaclarke/dev/engine/src/flutter/impeller/entity/contents/atlas_contents.cc:
/Users/aaclarke/dev/engine/src/flutter/impeller/entity/contents/atlas_contents.cc:12:10: error: 'impeller/entity/atlas_fill.frag.h' file not found [clang-diagnostic-error]
#include "impeller/entity/atlas_fill.frag.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/aaclarke/dev/engine/src/flutter/impeller/entity/contents/atlas_contents.cc:25:57: error: the parameter 'texture' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
void AtlasContents::SetTexture(std::shared_ptr<Texture> texture) {
                                                        ^
                               const                   &
/Users/aaclarke/dev/engine/src/flutter/impeller/entity/contents/atlas_contents.cc:73:60: error: the parameter 'desc' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
void AtlasContents::SetSamplerDescriptor(SamplerDescriptor desc) {
                                                           ^
                                         const            &
Suppressed 2465 warnings (2461 in non-user code, 4 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings treated as errors

jmagman
jmagman reviewed Oct 26, 2022
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

I think this would be testable?

engine/tools/clang_tidy/test/clang_tidy_test.dart

Lines 206 to 207 in e39c4df

final StringBuffer outBuffer = StringBuffer();
final StringBuffer errBuffer = StringBuffer();

zanderso
zanderso reviewed Oct 26, 2022
View reviewed changes
tools/clang_tidy/lib/clang_tidy.dart Outdated
return _ComputeJobsResult(jobs, sawMalformed);
}

Iterable<String> _trimGenerator(String output) sync* {
Copy link
Member

@zanderso zanderso Oct 26, 2022

Choose a reason for hiding this comment

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

You can test this by making it public, adding the @visibleForTesting annotation, and writing a unit test in clang_tidy/test/clang_tidy_test.dart

Copy link
Member Author

@gaaclarke gaaclarke Nov 1, 2022

Choose a reason for hiding this comment

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

done

@gaaclarke
Copy link
Member Author

gaaclarke commented Nov 1, 2022

test added

@gaaclarke gaaclarke requested review from jmagman and zanderso November 1, 2022 21:51
@gaaclarke gaaclarke force-pushed the clang-tidy-cleaner-output branch from 440f81c to 51cb278 Compare November 1, 2022 22:42
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 1, 2022
@auto-submit auto-submit bot merged commit 85c23bc into flutter:main Nov 2, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 2, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2022
@engine-flutter-autoroll
85c23bc Clang-tidy: cleaned up the output. (flutter/engine#37059)
7a3e997
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
@gaaclarke @schwa423
Clang-tidy: cleaned up the output. (flutter#37059)
eba55f3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jmagman jmagman jmagman approved these changes

@zanderso zanderso zanderso approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App needs tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gaaclarke @jmagman @zanderso