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

fix: add std=c++17 flag for e20+ #1022

Merged
merged 1 commit into from
Jul 29, 2022
Merged

fix: add std=c++17 flag for e20+ #1022

merged 1 commit into from
Jul 29, 2022

Conversation

VerteDinde
Copy link
Member

@VerteDinde VerteDinde commented Jul 6, 2022

Addresses e/e issue 34209 while 20.0.0-beta.1 is in beta.

This PR adds a compile flag for -std=c++17 when using Electron 20 or higher.

@malept
Copy link
Member

malept commented Jul 6, 2022

Is this a chore: or a fix:?

@VerteDinde VerteDinde changed the title chore: add std=c++17 flag for e20+ fix: add std=c++17 flag for e20+ Jul 6, 2022
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Might be worth adding a test if we have unit tests for clang-fetcher?

src/clang-fetcher.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2022

Codecov Report

Merging #1022 (979361d) into master (20107a8) will decrease coverage by 0.37%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #1022      +/-   ##
==========================================
- Coverage   76.38%   76.01%   -0.38%     
==========================================
  Files          19       19              
  Lines         686      692       +6     
  Branches      131      133       +2     
==========================================
+ Hits          524      526       +2     
- Misses        118      120       +2     
- Partials       44       46       +2     
Impacted Files Coverage Δ
src/module-type/node-gyp.ts 82.50% <0.00%> (-2.12%) ⬇️
src/clang-fetcher.ts 79.72% <50.00%> (-1.70%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20107a8...979361d. Read the comment docs.

@VerteDinde
Copy link
Member Author

@malept Looks like we don't have any unit tests for clang-fetcher - I'll see if I can add some

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

This is good for now and solves a real problem.

For electron/electron I'm working on some solution that would provide the build flags that come from upstream so that we don't have to hardcode a particular version. However in practice this is only going to change every few years (any predictions on when Chromium will jump to C++20?)

Also, upstream hardcodes this in several places too, e.g. https://chromium-review.googlesource.com/c/chromium/src/+/3306812 for one example.

👍 on tests but since there weren't any before, I wouldn't block on it

@electron-bot
Copy link

🎉 This PR is included in version 3.2.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants