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

chore: fix clang-tidy warnings #38079

Merged
merged 2 commits into from
Apr 26, 2023
Merged

chore: fix clang-tidy warnings #38079

merged 2 commits into from
Apr 26, 2023

Conversation

dsanders11
Copy link
Member

Description of Change

Fixes the current warnings from clang-tidy, other than modernize-use-nullptr which I'm disabling for now as there's too much noise. There's a couple false positives I had to silence with // NOLINT.

The goal is to add a clang-tidy CI job, but that will be done in a future PR.

For completeness, here is the clang-tidy output without the fixes:

Warnings 
../../electron/shell/common/gin_helper/trackable_object.cc:24:3: warning: 'operator int' must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
  operator int32_t() const { return id_; }
  ^
  explicit 

../../electron/shell/browser/api/electron_api_desktop_capturer.cc:246:35: warning: unnecessary temporary object created while calling emplace_back [modernize-use-emplace]
      window_sources.emplace_back(DesktopCapturer::Source{
                                  ^~~~~~~~~~~~~~~~~~~~~~~~
../../electron/shell/browser/api/electron_api_desktop_capturer.cc:260:11: warning: unnecessary temporary object created while calling emplace_back [modernize-use-emplace]
          DesktopCapturer::Source{list->GetSource(i), std::string()});
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

../../electron/shell/browser/ui/webui/accessibility_ui.cc:155:23: warning: use emplace_back instead of push_back [modernize-use-emplace]
    property_filters->push_back(ui::AXPropertyFilter(attribute, type));
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~               ~
                      emplace_back(
../../electron/shell/browser/usb/usb_chooser_context_factory.cc:18:27: warning: use '= default' to define a trivial destructor [modernize-use-equals-default]
UsbChooserContextFactory::~UsbChooserContextFactory() {}
                          ^                           ~~
                                                      = default;
../../electron/shell/browser/usb/usb_chooser_controller.cc:57:15: warning: argument name 'device' in comment does not match parameter name 'device_info' [bugprone-argument-comment]
  RunCallback(/*device=*/nullptr);
              ^
../../electron/shell/browser/usb/usb_chooser_controller.h:61:52: note: 'device_info' declared here
  void RunCallback(device::mojom::UsbDeviceInfoPtr device_info);
                                                   ^
../../electron/shell/browser/usb/usb_chooser_controller.cc:88:17: warning: argument name 'device' in comment does not match parameter name 'device_info' [bugprone-argument-comment]
    RunCallback(/*device=*/nullptr);
                ^
../../electron/shell/browser/usb/usb_chooser_controller.h:61:52: note: 'device_info' declared here
  void RunCallback(device::mojom::UsbDeviceInfoPtr device_info);
                                                   ^
../../electron/shell/browser/usb/usb_chooser_controller.cc:98:19: warning: argument name 'device' in comment does not match parameter name 'device_info' [bugprone-argument-comment]
      RunCallback(/*device=*/nullptr);
                  ^
../../electron/shell/browser/usb/usb_chooser_controller.h:61:52: note: 'device_info' declared here
  void RunCallback(device::mojom::UsbDeviceInfoPtr device_info);
                                                   ^
../../electron/shell/browser/usb/usb_chooser_controller.cc:134:17: warning: argument name 'port' in comment does not match parameter name 'device_info' [bugprone-argument-comment]
    RunCallback(/*port=*/nullptr);
                ^
../../electron/shell/browser/usb/usb_chooser_controller.h:61:52: note: 'device_info' declared here
  void RunCallback(device::mojom::UsbDeviceInfoPtr device_info);
                                                   ^

../../electron/shell/browser/api/electron_api_utility_process.cc:282:12: warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
    return 0;
           ^
           false

../../electron/shell/browser/api/electron_api_system_preferences_mac.mm:465:45: warning: 'p' used after it was moved [bugprone-use-after-move]
                                  std::move(p), std::move(err_msg)));
                                            ^
../../electron/shell/browser/api/electron_api_system_preferences_mac.mm:469:31: note: move occurred here
                              base::BindOnce(
                              ^
../../electron/shell/browser/api/electron_api_system_preferences_mac.mm:465:45: note: the use and move are unsequenced, i.e. there is no guarantee about the order in which they are evaluated
                                  std::move(p), std::move(err_msg)));
                                            ^
../../electron/shell/browser/api/electron_api_system_preferences_mac.mm:471:45: warning: 'p' used after it was moved [bugprone-use-after-move]
                                  std::move(p)));
                                            ^
../../electron/shell/browser/api/electron_api_system_preferences_mac.mm:463:31: note: move occurred here
                              base::BindOnce(
                              ^
../../electron/shell/browser/api/electron_api_system_preferences_mac.mm:471:45: note: the use and move are unsequenced, i.e. there is no guarantee about the order in which they are evaluated
                                  std::move(p)));
                                            ^
../../electron/shell/browser/ui/file_dialog_mac.mm:286:14: warning: use emplace_back instead of push_back [modernize-use-emplace]
      paths->push_back(base::FilePath(base::SysNSStringToUTF8([url path])));
             ^~~~~~~~~~~~~~~~~~~~~~~~~                                   ~
             emplace_back(
../../electron/shell/renderer/electron_autofill_agent.cc:49:15: warning: use emplace_back instead of push_back [modernize-use-emplace]
      labels->push_back(std::u16string());
              ^~~~~~~~~~~~~~~~~~~~~~~~~~
              emplace_back(

../../electron/shell/browser/api/gpu_info_enumerator.cc:11:42: warning: initializer for member 'value_stack_' is redundant [readability-redundant-member-init]
GPUInfoEnumerator::GPUInfoEnumerator() : value_stack_(), current_{} {}
                                         ^~~~~~~~~~~~~~
../../electron/shell/browser/api/gpu_info_enumerator.cc:11:58: warning: initializer for member 'current_' is redundant [readability-redundant-member-init]
GPUInfoEnumerator::GPUInfoEnumerator() : value_stack_(), current_{} {}
                                                         ^~~~~~~~~~~
../../electron/shell/browser/bluetooth/electron_bluetooth_delegate.cc:87:10: warning: constructing string from nullptr is undefined behaviour [bugprone-string-constructor]
  return nullptr;
         ^
../../electron/shell/browser/electron_permission_manager.cc:211:5: warning: 'details' used after it was moved [bugprone-use-after-move]
    details.Set("requestingUrl",
    ^
../../electron/shell/browser/electron_permission_manager.cc:215:26: note: move occurred here
                         base::Value(std::move(details)));
                         ^
../../electron/shell/browser/electron_permission_manager.cc:211:5: note: the use happens in a later loop iteration than the move
    details.Set("requestingUrl",
    ^
../../electron/shell/browser/file_select_helper.cc:74:7: warning: initializer for member 'select_file_dialog_' is redundant [readability-redundant-member-init]
      select_file_dialog_(),
      ^~~~~~~~~~~~~~~~~~~~~
../../electron/shell/browser/file_select_helper.cc:75:7: warning: initializer for member 'select_file_types_' is redundant [readability-redundant-member-init]
      select_file_types_(),
      ^~~~~~~~~~~~~~~~~~~~
../../electron/shell/browser/font_defaults.cc:111:3: warning: use range-based for loop instead [modernize-loop-convert]
  for (size_t i = 0; i < kFontDefaultsLength; ++i) {
  ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      (auto pref : kFontDefaults)
../../electron/shell/browser/net/asar/asar_url_loader.cc:169:28: warning: use std::make_unique instead [modernize-make-unique]
      readable_data_source.reset(new mojo::FilteredDataSource(
                          ~^~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                           = std::make_unique<mojo::FilteredDataSource>

Checklist

  • PR description included and stakeholders cc'd

Release Notes

Notes: none

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 21, 2023
@miniak
Copy link
Contributor

miniak commented Apr 23, 2023

@dsanders11 lint fails on these

shell/browser/api/electron_api_system_preferences.cc:20: (cpplint) Unknown NOLINT error category: modernize-use-equals-default [readability/nolint] [5]
shell/browser/api/electron_api_system_preferences.cc:27: (cpplint) Unknown NOLINT error category: modernize-use-equals-default [readability/nolint] [5]

@codebytere
Copy link
Member

@dsanders11

shell/browser/api/electron_api_system_preferences.cc:20:  (cpplint) Unknown NOLINT error category: modernize-use-equals-default  [readability/nolint] [5]
shell/browser/api/electron_api_system_preferences.cc:27:  (cpplint) Unknown NOLINT error category: modernize-use-equals-default  [readability/nolint] [5]
Total errors found: 2

two things failing looks like

@codebytere codebytere added the semver/patch backwards-compatible bug fixes label Apr 24, 2023
@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours and removed new-pr 🌱 PR opened in the last 24 hours labels Apr 24, 2023
jkleinsc
jkleinsc previously requested changes Apr 24, 2023
View reviewed changes
Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Lint needs to be fixed

@codebytere
Copy link
Member

@dsanders11 looks like it's confused by modernize-use-equals-default 🤔

@dsanders11
Copy link
Member Author

Lint fixed. cpplint apparently doesn't play nice with others and expects to find only its own categories specified for the NOLINT/NOLINTNEXTLINE comment, but clang-tidy also uses that comment syntax. Which is unfortunate.

Refactored the problematic code to remove the comment all together.

electron/shell/browser/api/electron_api_system_preferences.cc

Lines 20 to 34 in 225336e

#if BUILDFLAG(IS_WIN)
SystemPreferences::SystemPreferences() {
InitializeWindow();
}
#else
SystemPreferences::SystemPreferences() = default;
#endif
#if BUILDFLAG(IS_WIN)
SystemPreferences::~SystemPreferences() {
Browser::Get()->RemoveObserver(this);
}
#else
SystemPreferences::~SystemPreferences() = default;
#endif

@jkleinsc jkleinsc merged commit 08593fd into electron:main Apr 26, 2023
@release-clerk
Copy link

release-clerk bot commented Apr 26, 2023

No Release Notes

@dsanders11 dsanders11 deleted the clang-tidy-clean-run branch April 26, 2023 18:16
@commit-lint commit-lint bot mentioned this pull request Jul 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codebytere codebytere codebytere approved these changes

@miniak miniak miniak approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

Assignees
No one assigned
Labels
no-backport semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dsanders11 @miniak @codebytere @jkleinsc