Skip to content

tools: fix js2c regression #27980

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

Closed
refack wants to merge 2 commits into from
Closed

tools: fix js2c regression #27980

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b0295c
tools: fix js2c regression
refack May 30, 2019
b2d35a3
squash! wrap long line
refack Jun 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
tools: fix js2c regression
  • Loading branch information
@refack
refack committed Jun 1, 2019
commit 6b0295c1b0a73fd211ce4f3bb7e45db6884a37fe
1 change: 1 addition & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,7 @@
'test/cctest/test_node_postmortem_metadata.cc',
'test/cctest/test_environment.cc',
'test/cctest/test_linked_binding.cc',
'test/cctest/test_per_process.cc',
Comment thread
refack marked this conversation as resolved.
Outdated
'test/cctest/test_platform.cc',
'test/cctest/test_report_util.cc',
'test/cctest/test_traced_value.cc',
Expand Down
5 changes: 5 additions & 0 deletions src/node_native_module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
#include "node_union_bytes.h"
#include "v8.h"

// Forward declare test fixture for `friend` declaration.
class PerProcessTest;

namespace node {
namespace native_module {

Expand Down Expand Up @@ -82,6 +85,8 @@ class NativeModuleLoader {

// Used to synchronize access to the code cache map
Mutex code_cache_mutex_;

friend class ::PerProcessTest;
};
} // namespace native_module

Expand Down
34 changes: 34 additions & 0 deletions test/cctest/test_per_process.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#include "node_native_module.h"

#include "gtest/gtest.h"
#include "node_test_fixture.h"

#include <string>


using node::native_module::NativeModuleLoader;
using node::native_module::NativeModuleRecordMap;

class PerProcessTest : public ::testing::Test {
protected:
static const NativeModuleRecordMap get_sources_for_test() {
return NativeModuleLoader::instance_.source_;
Copy link
Copy Markdown
Member

@joyeecheung joyeecheung May 30, 2019

Choose a reason for hiding this comment

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

The builds are failing because this is private. I guess it would also make sense to add a public method to just return the source in a copy, though.

Copy link
Copy Markdown
Contributor Author

@refack refack May 30, 2019

Choose a reason for hiding this comment

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

I made it a friend
https://github.com/nodejs/node/blob/84aca5ca594f86ce9b6b86e6af039e78f90deaef/src/node_native_module.h#L89
and it compiles & passes with MSVC, now I need to figure out gcc & clang 🤷‍♂

Copy link
Copy Markdown
Contributor Author

@refack refack May 30, 2019

Choose a reason for hiding this comment

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

Seems like it was a namespace issue
https://github.com/nodejs/node/blob/c178598d309319e9a9e9692d6560a9205cd1b307/src/node_native_module.h#L89

}
};

namespace {

TEST_F(PerProcessTest, EmbeddedSources) {
const auto& sources = PerProcessTest::get_sources_for_test();
ASSERT_TRUE(
std::any_of(sources.cbegin(), sources.cend(),
[](auto p){ return p.second.is_one_byte(); }))
<< "NativeModuleLoader::source_ should have some 8bit items";

ASSERT_TRUE(
std::any_of(sources.cbegin(), sources.cend(),
[](auto p){ return !p.second.is_one_byte(); }))
<< "NativeModuleLoader::source_ should have some 16bit items";
}

} // end namespace
19 changes: 16 additions & 3 deletions tools/js2c.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,12 @@ def ReadMacros(macro_files):
}} // namespace node
"""

ONE_BYTE_STRING = """
static const uint8_t {0}[] = {{
{1}
}};
"""

TWO_BYTE_STRING = """
static const uint16_t {0}[] = {{
{1}
Expand All @@ -215,15 +221,22 @@ def ReadMacros(macro_files):
is_verbose = False

def GetDefinition(var, source, step=30):
encoded_source = bytearray(source, 'utf-16le')
code_points = [encoded_source[i] + (encoded_source[i+1] * 256) for i in range(0, len(encoded_source), 2)]
template = ONE_BYTE_STRING
code_points = [ord(c) for c in source]
if any(c > 127 for c in code_points):
template = TWO_BYTE_STRING
# Treat non-ASCII as UTF-8 and encode as UTF-16 Little Endian.
encoded_source = bytearray(source, 'utf-16le')
code_points = [encoded_source[i] + (encoded_source[i + 1] * 256) for i in range(0, len(encoded_source), 2)]
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jun 1, 2019

Choose a reason for hiding this comment

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

Can you wrap this, e.g. by putting the for i in ... on its own line, lining it up with the e in [encoded_source?

Copy link
Copy Markdown
Contributor Author

@refack refack Jun 1, 2019

Choose a reason for hiding this comment

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

Done.


# For easier debugging, align to the common 3 char for code-points.
elements_s = ['%3s' % x for x in code_points]
# Put no more then `step` code-points in a line.
slices = [elements_s[i:i + step] for i in range(0, len(elements_s), step)]
lines = [','.join(s) for s in slices]
array_content = ',\n'.join(lines)
definition = TWO_BYTE_STRING.format(var, array_content)
definition = template.format(var, array_content)

return definition, len(code_points)


Expand Down