FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: remove ERR prefix in WebCryptoKeyExportStatus by danbev · Pull Request #35639 · nodejs/node · GitHub

/ node Public

src: remove ERR prefix in WebCryptoKeyExportStatus - #35639

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:webcryptokeyexportstatus
ConversationCommits (1)ChecksFiles changed
Closed

src: remove ERR prefix in WebCryptoKeyExportStatus#35639
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:webcryptokeyexportstatus

Conversation

danbev commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

This commit suggests removing the ERR prefix in the
WebCryptoKeyExportStatus enum.

The motivation for this is that I think it improves the readability of
the code. For example, the following line had me look twice to see what
was going on:

  case WebCryptoKeyExportStatus::ERR_OK:
  // Success!
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

src: remove ERR prefix in WebCryptoKeyExportStatus
730f137 
This commit suggests removing the ERR prefix in the
WebCryptoKeyExportStatus enum.

The motivation for this is that I think it improves the readability of
the code. For example, the following line had me look twice to see what
was going on:

  case WebCryptoKeyExportStatus::ERR_OK:
  // Success!

nodejs-github-bot commented Oct 14, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/quic

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 14, 2020

nodejs-github-bot commented Oct 14, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33614/

jasnell approved these changes Oct 14, 2020
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I prefer the prefix but not enough to block ;)

danbev commented Oct 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I'm looking into the test-asan failure at the moment, but as far as I can tell it is not related to this issue:

cctest output 
$ out/Release/cctest --gtest_filter=EnvironmentTest.BufferWithFreeCallbackIsDetached
Running main() from ../test/cctest/gtest/gtest_main.cc
Note: Google Test filter = EnvironmentTest.BufferWithFreeCallbackIsDetached
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from EnvironmentTest
[ RUN      ] EnvironmentTest.BufferWithFreeCallbackIsDetached
6
=================================================================
==2773765==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x60400001af50 in thread T0:
  object passed to delete has wrong type:
  size of the allocated type:   48 bytes;
  size of the deallocated type: 1 bytes.
    #0 0x7f0a3a306175 in operator delete(void*, unsigned long) (/lib64/libasan.so.5+0x111175)
    #1 0x38328c2 in v8::internal::JSArrayBuffer::Detach(bool) (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x38328c2)
    #2 0x29bf88a in v8::ArrayBuffer::Detach() (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x29bf88a)
    #3 0x151adae in node::Buffer::(anonymous namespace)::CallbackInfo::CleanupHook(void*) (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x151adae)
    #4 0x144a5ff in node::Environment::RunCleanup() (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x144a5ff)
    #5 0x131a6f7 in node::FreeEnvironment(node::Environment*) (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x131a6f7)
    #6 0x11d7730 in EnvironmentTest_BufferWithFreeCallbackIsDetached_Test::TestBody() (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x11d7730)
    #7 0x1114cfd in testing::Test::Run() [clone .part.0] (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x1114cfd)
    #8 0x1115af0 in testing::TestInfo::Run() [clone .part.0] (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x1115af0)
    #9 0x11162c9 in testing::TestSuite::Run() [clone .part.0] (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x11162c9)
    #10 0x1137831 in testing::internal::UnitTestImpl::RunAllTests() (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x1137831)
    #11 0x11383bf in testing::UnitTest::Run() (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x11383bf)
    #12 0xa4d5b3 in main (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0xa4d5b3)
    #13 0x7f0a39ccf1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
    #14 0xa7f09d in _start (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0xa7f09d)

0x60400001af50 is located 0 bytes inside of 48-byte region [0x60400001af50,0x60400001af80)
allocated by thread T0 here:
    #0 0x7f0a3a304a97 in operator new(unsigned long) (/lib64/libasan.so.5+0x10fa97)
    #1 0x34cb1a8 in v8::internal::BackingStore::WrapAllocation(void*, unsigned long, void (*)(void*, unsigned long, void*), void*, v8::internal::SharedFlag) (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x34cb1a8)
    #2 0x29c18d5 in v8::ArrayBuffer::NewBackingStore(void*, unsigned long, void (*)(void*, unsigned long, void*), void*) (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x29c18d5)
    #3 0x1547193 in node::Buffer::New(node::Environment*, char*, unsigned long, void (*)(char*, void*), void*) (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x1547193)
    #4 0x1548e95 in node::Buffer::New(v8::Isolate*, char*, unsigned long, void (*)(char*, void*), void*) (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x1548e95)
    #5 0x11d7536 in EnvironmentTest_BufferWithFreeCallbackIsDetached_Test::TestBody() (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x11d7536)
    #6 0x1114cfd in testing::Test::Run() [clone .part.0] (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x1114cfd)
    #7 0x1115af0 in testing::TestInfo::Run() [clone .part.0] (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x1115af0)
    #8 0x11162c9 in testing::TestSuite::Run() [clone .part.0] (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x11162c9)
    #9 0x1137831 in testing::internal::UnitTestImpl::RunAllTests() (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x1137831)
    #10 0x11383bf in testing::UnitTest::Run() (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0x11383bf)
    #11 0xa4d5b3 in main (/home/danielbevenius/work/nodejs/node/out/Release/cctest+0xa4d5b3)
    #12 0x7f0a39ccf1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

SUMMARY: AddressSanitizer: new-delete-type-mismatch (/lib64/libasan.so.5+0x111175) in operator delete(void*, unsigned long)
==2773765==HINT: if you don't care about these errors you may set ASAN_OPTIONS=new_delete_type_mismatch=0
==2773765==ABORTING

jasnell commented Oct 15, 2020

Copy link
Copy Markdown
Member

@danbev I get those asan failures from time to time also. They definitely seem unrelated.

danbev commented Oct 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I get those asan failures from time to time also. They definitely seem unrelate

Thanks, I'm able to reproduce this consistently on my local machine so I'll take a closer look at it today.

I've collected some notes and might have a patch for this, but have run out of time today (and tomorrow is a Red Hat Recharge day so I won't be working). I'll continue with this when I get back on Monday. I've opened #35669 to track this.

danbev added a commit that referenced this pull request Oct 28, 2020
src: remove ERR prefix in WebCryptoKeyExportStatus
923f76d 
This commit suggests removing the ERR prefix in the
WebCryptoKeyExportStatus enum.

The motivation for this is that I think it improves the readability of
the code. For example, the following line had me look twice to see what
was going on:

  case WebCryptoKeyExportStatus::ERR_OK:
  // Success!

PR-URL: #35639
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

danbev commented Oct 28, 2020

Copy link
Copy Markdown
Contributor Author

Landed in 923f76d.

danbev closed this Oct 28, 2020
danbev deleted the webcryptokeyexportstatus branch October 28, 2020 05:17
targos pushed a commit that referenced this pull request Nov 3, 2020
src: remove ERR prefix in WebCryptoKeyExportStatus
c922578 
This commit suggests removing the ERR prefix in the
WebCryptoKeyExportStatus enum.

The motivation for this is that I think it improves the readability of
the code. For example, the following line had me look twice to see what
was going on:

  case WebCryptoKeyExportStatus::ERR_OK:
  // Success!

PR-URL: #35639
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos mentioned this pull request Nov 3, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL