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

src: do not set value on void return value #54449

Closed
wants to merge 1 commit into from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Aug 19, 2024

V8 added a static assertion to forbid it:

../../node/deps/v8/include/v8-function-callback.h:402:17: error: static assertion failed due to requirement '!std::is_void<void>::value': ReturnValue<void>::Set(const Local<S>) is deprecated. Do nothing to indicate that the operation succeeded or use SetFalse() to indicate that the operation failed (don't forget to handle info.ShouldThrowOnError()). See http://crbug.com/348660658 for details.
  402 |   static_assert(!std::is_void<T>::value,
      |                 ^~~~~~~~~~~~~~~~~~~~~~~
../../node/src/node_webstorage.cc:576:27: note: in instantiation of function template specialization 'v8::ReturnValue<void>::Set<v8::Value>' requested here
  576 |     info.GetReturnValue().Set(value);
      |                           ^

The assertion needs a newer version of clang to work so the official CI is not hit by it.

This commit also includes a change to GN file to fix GN build failure after V8 upgrade.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 19, 2024
@zcbenz zcbenz added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.32%. Comparing base (4f94397) to head (9b3436e).
Report is 31 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #54449   +/-   ##
=======================================
  Coverage   87.31%   87.32%           
=======================================
  Files         648      648           
  Lines      182321   182319    -2     
  Branches    34969    34969           
=======================================
+ Hits       159196   159210   +14     
+ Misses      16396    16385   -11     
+ Partials     6729     6724    -5     
Files Coverage Δ
src/node_webstorage.cc 73.51% <100.00%> (-0.14%) ⬇️

... and 16 files with indirect coverage changes

@nodejs-github-bot
Copy link
Collaborator

@zcbenz zcbenz requested a review from targos August 19, 2024 23:35
@avivkeller avivkeller added the v8 engine Issues and PRs related to the V8 dependency. label Aug 20, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Aug 24, 2024

I opened an alternative PR at #54418.

The assertion needs a newer version of clang to work so the official CI is not hit by it.

I don't think that's why official CI is not hit by it. The assertion is behind V8_IMMINENT_DEPRECATION_WARNINGS, which we don't set.

@zcbenz zcbenz closed this Aug 24, 2024
@zcbenz zcbenz deleted the no-set-void-return-value branch August 24, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants