| Summary: | [JSC] LLIntPrototypeLoadAdaptiveStructureWatchpoint does not require Bag<> | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin, ews-watchlist, rmorisset, saam, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Yusuke Suzuki
2019-05-06 21:37:59 PDT
Created attachment 369228 [details]
Patch
Comment on attachment 369228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369228&action=review LGTM overall. > Source/JavaScriptCore/ChangeLog:11 > + 1. Structure* and Instruction* are too large. We can just use StructureID and bytecodeOffsert (unsigned). offsert => offset Comment on attachment 369228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369228&action=review >> Source/JavaScriptCore/ChangeLog:11 >> + 1. Structure* and Instruction* are too large. We can just use StructureID and bytecodeOffsert (unsigned). > > offsert => offset Oops! Thanks. Comment on attachment 369228 [details] Patch Attachment 369228 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12120953 New failing tests: http/tests/misc/repeat-open-cancel.html Created attachment 369258 [details]
Archive of layout-test-results from ews213 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369228&action=review > Source/WTF/wtf/Nonmovable.h:29 > + private: \ Why private instead of just nothing? Comment on attachment 369228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369228&action=review >> Source/WTF/wtf/Nonmovable.h:29 >> + private: \ > > Why private instead of just nothing? This is aligned to NONCOPYABLE thing. I think this is because of the old implementation of NONCOPYABLE thing (putting copy constructor & assignment operator into private fields). I'll create the separate patch attempting to remove this "private". Committed r245050: <https://trac.webkit.org/changeset/245050> I think that both NONCOPYABLE and NONMOVABLE are a pattern that we should make obsolete. In new code where we need to emphasize that something is noncopyable we can simply write the constructor and assignment operator and use "= delete", something that was not practical back when we wrote Noncopyable.h. That would get rid of some strangeness with a macro that has "private:" in it, and also make things readable to C++ programmers that aren’t familiar with this particular WebKit macro. When we first wrote the file, the idiom was far less clear because of the lack of "= delete". Maybe others don’t feel the same way. But I, at least, would prefer to not have added NONMOVABLE and work to eventually get rid of NONCOPYABLE as well. (In reply to Darin Adler from comment #10) > I think that both NONCOPYABLE and NONMOVABLE are a pattern that we should > make obsolete. In new code where we need to emphasize that something is > noncopyable we can simply write the constructor and assignment operator and > use "= delete", something that was not practical back when we wrote > Noncopyable.h. > > That would get rid of some strangeness with a macro that has "private:" in > it, and also make things readable to C++ programmers that aren’t familiar > with this particular WebKit macro. > > When we first wrote the file, the idiom was far less clear because of the > lack of "= delete". > > Maybe others don’t feel the same way. But I, at least, would prefer to not > have added NONMOVABLE and work to eventually get rid of NONCOPYABLE as well. I like these macros because it allows you to write one line instead of two lines of code. That said, I don’t feel very strongly about it. (In reply to Saam Barati from comment #11) > (In reply to Darin Adler from comment #10) > > I think that both NONCOPYABLE and NONMOVABLE are a pattern that we should > > make obsolete. In new code where we need to emphasize that something is > > noncopyable we can simply write the constructor and assignment operator and > > use "= delete", something that was not practical back when we wrote > > Noncopyable.h. > > > > That would get rid of some strangeness with a macro that has "private:" in > > it, and also make things readable to C++ programmers that aren’t familiar > > with this particular WebKit macro. > > > > When we first wrote the file, the idiom was far less clear because of the > > lack of "= delete". > > > > Maybe others don’t feel the same way. But I, at least, would prefer to not > > have added NONMOVABLE and work to eventually get rid of NONCOPYABLE as well. > > I like these macros because it allows you to write one line instead of two > lines of code. That said, I don’t feel very strongly about it. Ah, what I'm saying is exploring whether we can drop this "private:" line :) (In reply to Yusuke Suzuki from comment #12) > (In reply to Saam Barati from comment #11) > > (In reply to Darin Adler from comment #10) > > > I think that both NONCOPYABLE and NONMOVABLE are a pattern that we should > > > make obsolete. In new code where we need to emphasize that something is > > > noncopyable we can simply write the constructor and assignment operator and > > > use "= delete", something that was not practical back when we wrote > > > Noncopyable.h. > > > > > > That would get rid of some strangeness with a macro that has "private:" in > > > it, and also make things readable to C++ programmers that aren’t familiar > > > with this particular WebKit macro. > > > > > > When we first wrote the file, the idiom was far less clear because of the > > > lack of "= delete". > > > > > > Maybe others don’t feel the same way. But I, at least, would prefer to not > > > have added NONMOVABLE and work to eventually get rid of NONCOPYABLE as well. > > > > I like these macros because it allows you to write one line instead of two > > lines of code. That said, I don’t feel very strongly about it. > > Ah, what I'm saying is exploring whether we can drop this "private:" line :) Oops, I missed the comment from Darin. Personally, I like the current NONCOPYABLE too, since it is easy to see and understand the intent :) |