-
Notifications
You must be signed in to change notification settings - Fork 34
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
Upgrade dependencies and CI config #804
Conversation
Codecov Report
@@ Coverage Diff @@
## master #804 +/- ##
==========================================
- Coverage 99.14% 99.02% -0.13%
==========================================
Files 80 80
Lines 12374 12789 +415
==========================================
+ Hits 12268 12664 +396
- Misses 106 125 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
|
They were added to order jobs in parallel-limited CI, but we now have 30 parallel jobs available.
9a6831a
to
602e627
Compare
@@ -100,4 +100,4 @@ static void parse_string(benchmark::State& state) | |||
|
|||
state.SetItemsProcessed(static_cast<int64_t>(size)); | |||
} | |||
BENCHMARK(parse_string)->RangeMultiplier(2)->Range(16, 4 * 1024); | |||
BENCHMARK(parse_string)->RangeMultiplier(2)->Range(16, 4096); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change needed after -bugprone-implicit-widening-of-multiplication-result
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change then?
@@ -124,7 +124,7 @@ WasmEngine::Result Wasm3Engine::execute( | |||
{ | |||
unsigned ret_valid; | |||
uint64_t ret_value; | |||
IM3Function function = reinterpret_cast<IM3Function>(func_ref); | |||
auto function = reinterpret_cast<IM3Function>(func_ref); // NOLINT(performance-no-int-to-ptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably new clang-tidy warning (as I upgraded to clang-tidy-13). Pointer to integer cast blocks some optimizations. https://clang.llvm.org/extra/clang-tidy/checks/performance-no-int-to-ptr.html.
lib/fizzy/instantiate.cpp
Outdated
@@ -203,7 +203,7 @@ std::tuple<bytes_ptr, Limits> allocate_memory(const std::vector<Memory>& module_ | |||
} | |||
|
|||
// NOTE: fill it with zeroes | |||
bytes_ptr memory{new bytes(memory_min * PageSize, 0), bytes_delete}; | |||
bytes_ptr memory{new bytes(uint32_t{memory_min * PageSize}, 0), bytes_delete}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gumb0 can you double check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is non-functional change. The uint32_t{}
explicitly states that the multiplication is done with 32-bit precision. This will not compile if any of multiplication operands has bigger type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change still needed with disabled bugprone-implicit-widening-of-multiplication-result
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not. I started "fixing" these but it turned out there are too many cases for constant multiplication. I reported the bug about the check but it is not worth it in current form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well turns out we have a bug on master here, because trying to allocate 4Gb actually overflows and allocates 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uint32_t{}
is my proposed why of silencing the warning. I can revert these 2 changes from this PR, especially if there is a fix now in a side PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would revert them if warning is disabled anyway.
@@ -66,7 +66,6 @@ if(CMAKE_CXX_COMPILER_ID MATCHES Clang) | |||
-Wno-double-promotion | |||
-Wno-float-equal | |||
-Wno-padded | |||
-Wno-return-std-move-in-c++11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was this causing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flag does not exist any more.
@@ -1 +1,2 @@ | |||
mut | |||
crate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not sure why, it's a normal English word?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. The codespell suggests crate -> create.
No description provided.