Add cheat sheet comment for HashMap/Set iterator/AddResult
Created attachment 374736 [details] Patch
Comment on attachment 374736 [details] Patch yes, awesome, i love it, 10/10, r=me :)
Comment on attachment 374736 [details] Patch Bad style comments. Read guide
(In reply to Daniel Bates from comment #3) > Comment on attachment 374736 [details] > Patch > > Bad style comments. Read guide Can you clarify? I don't see anything wrong according to: https://webkit.org/code-style-guidelines/
(In reply to Keith Miller from comment #4) > (In reply to Daniel Bates from comment #3) > > Comment on attachment 374736 [details] > > Patch > > > > Bad style comments. Read guide > > Can you clarify? I don't see anything wrong according to: > https://webkit.org/code-style-guidelines/ 😮 I thought there was a rule, no C-style comments with the unwritten exemptions being 1. License block 2. The few pure C headers we have to maintain 3. In parameter lists to document unused params But there's nothing. So, you're "free" to do as you wish. In my opinion I would use C++ style comments because of ^^^.
(In reply to Daniel Bates from comment #5) > (In reply to Keith Miller from comment #4) > > (In reply to Daniel Bates from comment #3) > > > Comment on attachment 374736 [details] > > > Patch > > > > > > Bad style comments. Read guide > > > > Can you clarify? I don't see anything wrong according to: > > https://webkit.org/code-style-guidelines/ > > 😮 I thought there was a rule, no C-style comments with the unwritten > exemptions being 1. License block 2. The few pure C headers we have to > maintain 3. In parameter lists to document unused params > > But there's nothing. So, you're "free" to do as you wish. In my opinion I > would use C++ style comments because of ^^^. Oh, I thought C supported // comments? Anywho, I kinda like going with the multi-line comments because that's the same style we use for API documentation in our public headers.
Comment on attachment 374736 [details] Patch Clearing flags on attachment: 374736 Committed r247767: <https://trac.webkit.org/changeset/247767>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53480775>
We now also support c++17 restructuring, so in many cases, you don't even need the temporary. For instance, instead of: auto addResult = hashset.add(valueToAdd); auto iter = addResult.iterator; bool isNewEntry = addResult.isNewEntry; You can now write: auto [iter, isNewEntry] = hashset.add(valueToAdd);
(In reply to Sam Weinig from comment #10) > We now also support c++17 restructuring, so in many cases, you don't even > need the temporary. For instance, instead of: > > > auto addResult = hashset.add(valueToAdd); > auto iter = addResult.iterator; > bool isNewEntry = addResult.isNewEntry; > > > You can now write: > > > auto [iter, isNewEntry] = hashset.add(valueToAdd); Whaaa?!? That's a feature?
(In reply to Sam Weinig from comment #10) > We now also support c++17 restructuring, so in many cases, you don't even > need the temporary. For instance, instead of: > > > auto addResult = hashset.add(valueToAdd); > auto iter = addResult.iterator; > bool isNewEntry = addResult.isNewEntry; > > > You can now write: > > > auto [iter, isNewEntry] = hashset.add(valueToAdd); wow. Very nice. This just matches order of fields in a struct?
(In reply to Saam Barati from comment #12) > (In reply to Sam Weinig from comment #10) > > We now also support c++17 restructuring, so in many cases, you don't even > > need the temporary. For instance, instead of: > > > > > > auto addResult = hashset.add(valueToAdd); > > auto iter = addResult.iterator; > > bool isNewEntry = addResult.isNewEntry; > > > > > > You can now write: > > > > > > auto [iter, isNewEntry] = hashset.add(valueToAdd); > > wow. Very nice. This just matches order of fields in a struct? Yup. https://en.cppreference.com/w/cpp/language/structured_binding. Another fun things you can do with it: for (auto& [key, value] : myHashMap) { ... }
(In reply to Sam Weinig from comment #13) > (In reply to Saam Barati from comment #12) > > (In reply to Sam Weinig from comment #10) > > > We now also support c++17 restructuring, so in many cases, you don't even > > > need the temporary. For instance, instead of: > > > > > > > > > auto addResult = hashset.add(valueToAdd); > > > auto iter = addResult.iterator; > > > bool isNewEntry = addResult.isNewEntry; > > > > > > > > > You can now write: > > > > > > > > > auto [iter, isNewEntry] = hashset.add(valueToAdd); > > > > wow. Very nice. This just matches order of fields in a struct? > > Yup. https://en.cppreference.com/w/cpp/language/structured_binding. > > Another fun things you can do with it: > > for (auto& [key, value] : myHashMap) { > ... > } I started using it recently with: https://bugs.webkit.org/show_bug.cgi?id=198905 https://bugs.webkit.org/show_bug.cgi?id=199247
(In reply to Sam Weinig from comment #14) > (In reply to Sam Weinig from comment #13) > > (In reply to Saam Barati from comment #12) > > > (In reply to Sam Weinig from comment #10) > > > > We now also support c++17 restructuring, so in many cases, you don't even > > > > need the temporary. For instance, instead of: > > > > > > > > > > > > auto addResult = hashset.add(valueToAdd); > > > > auto iter = addResult.iterator; > > > > bool isNewEntry = addResult.isNewEntry; > > > > > > > > > > > > You can now write: > > > > > > > > > > > > auto [iter, isNewEntry] = hashset.add(valueToAdd); > > > > > > wow. Very nice. This just matches order of fields in a struct? > > > > Yup. https://en.cppreference.com/w/cpp/language/structured_binding. > > > > Another fun things you can do with it: > > > > for (auto& [key, value] : myHashMap) { > > ... > > } > > I started using it recently with: > https://bugs.webkit.org/show_bug.cgi?id=198905 > https://bugs.webkit.org/show_bug.cgi?id=199247 That's really nice.
I guess one area that could be worrisome is this leaks field order of structs in an unfortunate way. Now, if you change a struct with all public fields, you need to ensure nobody is using structured binding of it.
(In reply to Saam Barati from comment #16) > I guess one area that could be worrisome is this leaks field order of > structs in an unfortunate way. Now, if you change a struct with all public > fields, you need to ensure nobody is using structured binding of it. Yeah, that could be a concern for sure. I think restricting use to idiomatic places (e.g. HashMap iteration, HashMap/HashSet AddResult, std::pair, std::tuple, etc) should help us here. Also, if you do change the struct order and want to preserve the old destructing order, you actually can by overriding std::tuple_size, std::tuple_element and std::get<> for the type. https://blog.tartanllama.xyz/structured-bindings/ has some good information on that. Of course, this doesn't help at all if you don't know people are using structured bindings.