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

Fix a bunch of chromium-style errors #12647

Merged
merged 10 commits into from Apr 19, 2018

Conversation

Projects
None yet
5 participants
@nornagon
Contributor

nornagon commented Apr 18, 2018

We're not yet running any of the chromium-style checkers on our build, but as
part of exploring porting our build system to GN, I happened across a few
places where the chromium-style clang plugins threw warnings. This fixes them.

These patches are separated out by warning type, so I recommend reading each
commit separately, rather than using the "Files Changed" tab.

See here for more information about the chromium-style checker.

@nornagon nornagon requested review from electron/browserview as code owners Apr 18, 2018

@zcbenz zcbenz referenced this pull request Apr 18, 2018

Merged

clang-format atom files #12648

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Apr 18, 2018

Member

We're not yet running any of the chromium-style checkers on our build

Have enabled the stye checker plugin from the downloaded clang prebuilt, the config is based on https://cs.chromium.org/chromium/src/build/config/clang/BUILD.gn

Member

deepak1556 commented Apr 18, 2018

We're not yet running any of the chromium-style checkers on our build

Have enabled the stye checker plugin from the downloaded clang prebuilt, the config is based on https://cs.chromium.org/chromium/src/build/config/clang/BUILD.gn

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Apr 18, 2018

Member

Seems like there is a style-error in the upstream source,

/Users/distiller/project/vendor/download/libchromiumcontent/src/content/public/browser/cookie_store_factory.h:32:1: error: [chromium-style] Complex class/struct needs an explicit out-of-line copy constructor.
struct CONTENT_EXPORT CookieStoreConfig {
^
1 error generated.
ninja: build stopped: subcommand failed.
Member

deepak1556 commented Apr 18, 2018

Seems like there is a style-error in the upstream source,

/Users/distiller/project/vendor/download/libchromiumcontent/src/content/public/browser/cookie_store_factory.h:32:1: error: [chromium-style] Complex class/struct needs an explicit out-of-line copy constructor.
struct CONTENT_EXPORT CookieStoreConfig {
^
1 error generated.
ninja: build stopped: subcommand failed.
@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Apr 18, 2018

Contributor

Seems like there is a style-error in the upstream source

Can we disable this particular check in the config?

Contributor

alexeykuzmin commented Apr 18, 2018

Seems like there is a style-error in the upstream source

Can we disable this particular check in the config?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Apr 18, 2018

Member

Can we disable this particular check in the config?

Its not possible.

Also note that copying a complex class can be an expensive operation. The preferred solution is almost always is to avoid the copy if possible. Please consider changing the code and eliminating unnecessary copies instead of simply adding an out of line copy constructor to silence the error. Additionally, in a large number of cases, you can utilize move semantics to improve the efficiency of the code.

From the chromium documentation, the right fix here would be avoid generating a copy of that class. I am working on the fix.

Member

deepak1556 commented Apr 18, 2018

Can we disable this particular check in the config?

Its not possible.

Also note that copying a complex class can be an expensive operation. The preferred solution is almost always is to avoid the copy if possible. Please consider changing the code and eliminating unnecessary copies instead of simply adding an out of line copy constructor to silence the error. Additionally, in a large number of cases, you can utilize move semantics to improve the efficiency of the code.

From the chromium documentation, the right fix here would be avoid generating a copy of that class. I am working on the fix.

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Apr 18, 2018

Member

@deepak1556 could this be enabled for Linux as well with something like this?

+                'cflags_cc': [
+                  '-Xclang', '-load',
+                  '-Xclang', '<(source_root)/<(make_clang_dir)/lib/libFindBadConstructs.so',
+                  '-Xclang', '-add-plugin',
+                  '-Xclang', 'find-bad-constructs',
+                  '-Xclang', '-plugin-arg-find-bad-constructs',
+                  '-Xclang', 'check-auto-raw-pointer'
+                ],
Member

ckerr commented Apr 18, 2018

@deepak1556 could this be enabled for Linux as well with something like this?

+                'cflags_cc': [
+                  '-Xclang', '-load',
+                  '-Xclang', '<(source_root)/<(make_clang_dir)/lib/libFindBadConstructs.so',
+                  '-Xclang', '-add-plugin',
+                  '-Xclang', 'find-bad-constructs',
+                  '-Xclang', '-plugin-arg-find-bad-constructs',
+                  '-Xclang', 'check-auto-raw-pointer'
+                ],
@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Apr 18, 2018

Member

@ckerr yup that should work, I was planning to do it once the mac build got fixed, Feel free to add it. Thanks!

Member

deepak1556 commented Apr 18, 2018

@ckerr yup that should work, I was planning to do it once the mac build got fixed, Feel free to add it. Thanks!

@nornagon nornagon requested a review from electron/reviewers Apr 19, 2018

@zcbenz

zcbenz approved these changes Apr 19, 2018

@zcbenz zcbenz merged commit 55e2dbd into electron:master Apr 19, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment