Stop using operator[] only for default-insertion into maps #8129

dschuff · 2025-12-15T21:23:04Z

Upstream libc++ recently added the [[nodiscard]] attribute to the return value
of operator[] on maps, meaning the compiler now warns on uses of this operator
that only want to insert rather than also use the result.

Switch to using try_emplace, which does not construct the value unless insertion
succeeds, and is IMO more readable than operator[] for this purpose.

Fixes #8124

Upstream libc++ recently added the [[nodiscard]] attribute to the return value of operator[] on maps, meaning the compiler now warns on uses of this operator that only want to insert rather than also use the result. Switch to using try_emplace, which does not construct the value unless insertion succeeds, and is more readable than operator[] for this purpose.

tlively

Can we add this warning on CI to avoid regressions?

tlively · 2025-12-15T21:30:39Z

src/ir/module-utils.h

    // entry).
    for (auto& func : wasm.functions) {
-      map[func.get()];
+      map.insert({func.get(), {}});


Why insert instead of try_emplace here?

Ah right I wanted to mention that. It's because map is actually an instance of wasm::InsertOrderedMap, (https://github.com/WebAssembly/binaryen/blob/main/src/support/insert_ordered.h#L92) and doesn't have try_emplace. So the other option would be to just fix that instead.

dschuff · 2025-12-15T21:39:07Z

Can we add this warning on CI to avoid regressions?

I don't think we can without using the bleeding-edge libc++ because the [[nodiscard]] attribute isn't there before then, so there's nothing to warn about. I wouldn't actually be surprised if the change ends up getting backed out because it catches a lot of this code (there's already discussion on the relevant PR, see the link in the bug). But this still seems like a readability improvement.

kripken · 2025-12-15T21:42:52Z

src/passes/Inlining.cpp

    // entry)
    for (auto& func : module->functions) {
-      infos[func->name];
+      infos.try_emplace(func->name);


These will definitely insert (the map is empty before), so perhaps emplace would be faster than try_emplace..? I admit I don't know if this would matter though.

The same might happen in a few other places, but this one is especially clear-cut (we clear the map right before the loop).

Yeah, I thought about that. The other difference between emplace and try_emplace is that the value is constructed with std::piecewise_construct, and that actually matters, for reasons I don't 100% understand. For example, this spot. Here's the error if you try to use emplace here:

[build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/new_allocator.h:191:23: error: no matching constructor for initialization of 'std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>' [build] 191 | { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); } [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/alloc_traits.h:575:8: note: in instantiation of function template specialization 'std::__new_allocator<std::__detail::_Hash_node<std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, true>>::construct<std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, wasm::Name &>' requested here [build] 575 | __a.construct(__p, std::forward<_Args>(__args)...); [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/hashtable_policy.h:2030:27: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<std::__detail::_Hash_node<std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, true>>>::construct<std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, wasm::Name &>' requested here [build] 2030 | __node_alloc_traits::construct(__alloc, __n->_M_valptr(), [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/hashtable.h:312:19: note: in instantiation of function template specialization 'std::__detail::_Hashtable_alloc<std::allocator<std::__detail::_Hash_node<std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, true>>>::_M_allocate_node<wasm::Name &>' requested here [build] 312 | _M_node(__h->_M_allocate_node(std::forward<_Args>(__args)...)) [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/hashtable.h:2147:15: note: in instantiation of function template specialization 'std::_Hashtable<wasm::Name, std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, std::allocator<std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>>, std::__detail::_Select1st, std::equal_to<wasm::Name>, std::hash<wasm::Name>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true>>::_Scoped_node::_Scoped_node<wasm::Name &>' requested here [build] 2147 | _Scoped_node __node { this, std::forward<_Args>(__args)... }; [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/hashtable.h:1004:11: note: in instantiation of function template specialization 'std::_Hashtable<wasm::Name, std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, std::allocator<std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>>, std::__detail::_Select1st, std::equal_to<wasm::Name>, std::hash<wasm::Name>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true>>::_M_emplace<wasm::Name &>' requested here [build] 1004 | { return _M_emplace(__unique_keys{}, std::forward<_Args>(__args)...); } [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/unordered_map.h:396:16: note: in instantiation of function template specialization 'std::_Hashtable<wasm::Name, std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, std::allocator<std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>>, std::__detail::_Select1st, std::equal_to<wasm::Name>, std::hash<wasm::Name>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true>>::emplace<wasm::Name &>' requested here [build] 396 | { return _M_h.emplace(std::forward<_Args>(__args)...); } [build] | ^ [build] /usr/local/google/home/dschuff/s/emr/emscripten-releases/binaryen/src/passes/Inlining.cpp:1347:13: note: in instantiation of function template specialization 'std::unordered_map<wasm::Name, wasm::(anonymous namespace)::FunctionInfo>::emplace<wasm::Name &>' requested here [build] 1347 | infos.emplace(func->name); [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:294:17: note: candidate constructor not viable: no known conversion from 'wasm::Name' to 'const pair<const Name, FunctionInfo>' for 1st argument [build] 294 | constexpr pair(const pair&) = default; ///< Copy constructor [build] | ^ ~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:779:12: note: candidate template ignored: could not match 'pair<_U1, _U2>' against 'wasm::Name' [build] 779 | constexpr pair(const pair<_U1, _U2>& __p) [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:789:21: note: candidate template ignored: could not match 'pair<_U1, _U2>' against 'wasm::Name' [build] 789 | explicit constexpr pair(const pair<_U1, _U2>& __p) [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:902:12: note: candidate template ignored: could not match 'pair<_U1, _U2>' against 'wasm::Name' [build] 902 | constexpr pair(pair<_U1, _U2>&& __p) [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:913:21: note: candidate template ignored: could not match 'pair<_U1, _U2>' against 'wasm::Name' [build] 913 | explicit constexpr pair(pair<_U1, _U2>&& __p) [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:725:17: note: candidate constructor template not viable: requires 0 arguments, but 1 was provided [build] 725 | constexpr pair() [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:737:26: note: candidate constructor template not viable: requires 0 arguments, but 1 was provided [build] 737 | explicit constexpr pair() [build] | ^ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:752:17: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided [build] 752 | constexpr pair(const _T1& __a, const _T2& __b) [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:762:26: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided [build] 762 | explicit constexpr pair(const _T1& __a, const _T2& __b) [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:830:2: note: candidate constructor template not viable: requires at least 2 arguments, but 1 was provided [build] 830 | pair(_U1&& __x, __zero_as_null_pointer_constant, ...) [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:843:2: note: candidate constructor template not viable: requires at least 2 arguments, but 1 was provided [build] 843 | pair(_U1&& __x, __zero_as_null_pointer_constant, ...) [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:856:2: note: candidate constructor template not viable: requires at least 2 arguments, but 1 was provided [build] 856 | pair(__zero_as_null_pointer_constant, _U2&& __y, ...) [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:869:2: note: candidate constructor template not viable: requires at least 2 arguments, but 1 was provided [build] 869 | pair(__zero_as_null_pointer_constant, _U2&& __y, ...) [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:881:12: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided [build] 881 | constexpr pair(_U1&& __x, _U2&& __y) [build] | ^ ~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:891:21: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided [build] 891 | explicit constexpr pair(_U1&& __x, _U2&& __y) [build] | ^ ~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:299:2: note: candidate constructor template not viable: requires 3 arguments, but 1 was provided [build] 299 | pair(piecewise_construct_t, tuple<_Args1...>, tuple<_Args2...>); [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_pair.h:335:2: note: candidate constructor template not viable: requires 4 arguments, but 1 was provided [build] 335 | pair(tuple<_Args1...>&, tuple<_Args2...>&, [build] | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [build] 336 | _Index_tuple<_Indexes1...>, _Index_tuple<_Indexes2...>); [build] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Ok, I think it's because emplace forwards all of the arguments to the constructor of the underlying pair (e.g. std::pair<const wasm::Name, wasm::(anonymous namespace)::FunctionInfo>, and we are passing just the key, but a std::pair can't be constructed from just the first element. However try_emplace takes its first argument explicitly as the key, and forwards the rest of its arguments (none in this case) to construct the value. So it default-constructs the value, and creates the pair from the explicit key and the default-constructed value, and succeeds.

I see, sounds good.

lgtm

tlively

LGTM. I do think the code is more readable this way anyway.

dschuff requested review from kripken and tlively and removed request for tlively December 15, 2025 21:23

tlively reviewed Dec 15, 2025

View reviewed changes

kripken reviewed Dec 15, 2025

View reviewed changes

tlively approved these changes Dec 15, 2025

View reviewed changes

dschuff merged commit 16f7f8d into main Dec 16, 2025
17 checks passed

dschuff deleted the nodiscard branch December 16, 2025 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stop using operator[] only for default-insertion into maps #8129

Stop using operator[] only for default-insertion into maps #8129

Uh oh!

dschuff commented Dec 15, 2025 •

edited

Loading

Uh oh!

tlively left a comment

Uh oh!

tlively Dec 15, 2025

Uh oh!

dschuff Dec 15, 2025

Uh oh!

dschuff commented Dec 15, 2025

Uh oh!

kripken Dec 15, 2025

Uh oh!

dschuff Dec 15, 2025 •

edited

Loading

Uh oh!

dschuff Dec 15, 2025 •

edited

Loading

Uh oh!

kripken Dec 15, 2025

Uh oh!

tlively left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Stop using operator[] only for default-insertion into maps #8129

Stop using operator[] only for default-insertion into maps #8129

Uh oh!

Conversation

dschuff commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

tlively Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

dschuff Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

dschuff commented Dec 15, 2025

Uh oh!

kripken Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

dschuff Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dschuff Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dschuff commented Dec 15, 2025 •

edited

Loading

dschuff Dec 15, 2025 •

edited

Loading

dschuff Dec 15, 2025 •

edited

Loading