RESOLVED FIXED 200061
Add cheat sheet comment for HashMap/Set iterator/AddResult
https://bugs.webkit.org/show_bug.cgi?id=200061
Summary Add cheat sheet comment for HashMap/Set iterator/AddResult
Keith Miller
Reported 2019-07-23 16:54:08 PDT
Add cheat sheet comment for HashMap/Set iterator/AddResult
Attachments
Patch (2.70 KB, patch)
2019-07-23 16:54 PDT, Keith Miller
no flags
Keith Miller
Comment 1 2019-07-23 16:54:22 PDT
Devin Rousso
Comment 2 2019-07-23 17:07:03 PDT
Comment on attachment 374736 [details] Patch yes, awesome, i love it, 10/10, r=me :)
Daniel Bates
Comment 3 2019-07-23 19:20:03 PDT
Comment on attachment 374736 [details] Patch Bad style comments. Read guide
Keith Miller
Comment 4 2019-07-23 19:26:51 PDT
(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/
Daniel Bates
Comment 5 2019-07-23 20:24:00 PDT
(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 ^^^.
Keith Miller
Comment 6 2019-07-23 20:59:03 PDT
(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.
WebKit Commit Bot
Comment 7 2019-07-23 21:29:25 PDT
Comment on attachment 374736 [details] Patch Clearing flags on attachment: 374736 Committed r247767: <https://trac.webkit.org/changeset/247767>
WebKit Commit Bot
Comment 8 2019-07-23 21:29:27 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 9 2019-07-23 21:38:04 PDT
Sam Weinig
Comment 10 2019-07-24 11:01:52 PDT
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);
Keith Miller
Comment 11 2019-07-24 11:02:36 PDT
(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?
Saam Barati
Comment 12 2019-07-24 13:10:11 PDT
(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?
Sam Weinig
Comment 13 2019-07-24 13:22:13 PDT
(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) { ... }
Sam Weinig
Comment 14 2019-07-24 13:24:41 PDT
(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
Saam Barati
Comment 15 2019-07-24 14:54:44 PDT
(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.
Saam Barati
Comment 16 2019-07-24 14:59:20 PDT
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.
Sam Weinig
Comment 17 2019-07-24 16:31:04 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.