WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2019-07-23 16:54:22 PDT
Created
attachment 374736
[details]
Patch
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
<
rdar://problem/53480775
>
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.
Top of Page
Format For Printing
XML
Clone This Bug