| Summary: | Streamline MarkupAccumulator to improve efficiency a bit | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||
| Component: | HTML Editing | Assignee: | Darin Adler <darin> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | andersca, benjamin, cdumez, changseok, cmarcelo, dbarton, dino, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gyuyoung.kim, keith_miller, kondapallykalyan, mark.lam, mifenton, mmaxfield, msaboff, pdr, saam, sabouhallawa, sam, schenney, sergio, tzagallo, webkit-bug-importer, wenson_hsieh | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Darin Adler
2020-05-08 19:27:26 PDT
Created attachment 398910 [details]
Patch
Comment on attachment 398910 [details]
Patch
Lost a quote mark. Oops.
Created attachment 398925 [details]
Patch
Created attachment 398926 [details]
Patch
Comment on attachment 398926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398926&action=review > Source/WebCore/editing/MarkupAccumulator.h:75 > + template<typename ...StringTypes> void append(StringTypes... strings) { m_markup.append(strings...); } Any reason why StringTypes isn't a forwarding reference, and why you're not using std::forward in the call? Comment on attachment 398926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398926&action=review >> Source/WebCore/editing/MarkupAccumulator.h:75 >> + template<typename ...StringTypes> void append(StringTypes... strings) { m_markup.append(strings...); } > > Any reason why StringTypes isn't a forwarding reference, and why you're not using std::forward in the call? No reason. I will do what you suggest. Also, I will need to fix the crash seen on mac-debug! Ah, got the null check wrong. Committed r261437: <https://trac.webkit.org/changeset/261437> Comment on attachment 398926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398926&action=review > Source/WebCore/editing/markup.cpp:407 > +// Stopgap until C++20 adds std::ranges::reverse_view. > +template<typename Collection> struct ReverseView { > + Collection& collection; > + decltype(collection.rbegin()) begin() const { return collection.rbegin(); } > + decltype(collection.rend()) end() const { return collection.rend(); } > + decltype(collection.size()) size() const { return collection.size(); } > + ReverseView(Collection& collection) > + : collection(collection) > + { > + } > +}; We really should just import a copy of our own as we did with Optional and Variant. That said, we actually have the equivalent (I think) functionality already by way of makeReversedRange() in <wtf/IteratorRange.h>. (In reply to Sam Weinig from comment #11) > We really should just import a copy of our own as we did with Optional and > Variant. OK. I was just working on replacing WTF::Optional with std::optional. > That said, we actually have the equivalent (I think) functionality already > by way of makeReversedRange() in <wtf/IteratorRange.h>. I’ll try using that. (In reply to Darin Adler from comment #12) > (In reply to Sam Weinig from comment #11) > > We really should just import a copy of our own as we did with Optional and > > Variant. > > OK. > > I was just working on replacing WTF::Optional with std::optional. Sure. That's the right thing to do now. All I meant that I we brought those in before we had a copy in the standard library we could use. (In reply to Sam Weinig from comment #13) > (In reply to Darin Adler from comment #12) > > I was just working on replacing WTF::Optional with std::optional. > > Sure. That's the right thing to do now. All I meant that I we brought those > in before we had a copy in the standard library we could use. Sure, I understood that. I was sort of observing the flow of the tides in and back out. |