Bug 200061 – Add cheat sheet comment for HashMap/Set iterator/AddResult

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Add cheat sheet comment for HashMap/Set iterator/AddResult

[Keith Miller]

Created attachment 374736 [details]
Patch

[Devin Rousso]

Comment on attachment 374736 [details]
Patch

yes, awesome, i love it, 10/10, r=me :)

[Daniel Bates]

Comment on attachment 374736 [details]
Patch

Bad style comments. Read guide

[Keith Miller]

(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]

(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]

(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 on attachment 374736 [details]
Patch

Clearing flags on attachment: 374736

Committed r247767: <https://trac.webkit.org/changeset/247767>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53480775>

[Sam Weinig]

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]

(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]

(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]

(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]

(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]

(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]

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]

(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.