Bug 197645 - [JSC] LLIntPrototypeLoadAdaptiveStructureWatchpoint does not require Bag<>
Summary: [JSC] LLIntPrototypeLoadAdaptiveStructureWatchpoint does not require Bag<>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-06 21:37 PDT by Yusuke Suzuki
Modified: 2019-05-09 15:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.62 KB, patch)
2019-05-06 21:45 PDT, Yusuke Suzuki
sbarati: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.63 MB, application/zip)
2019-05-07 02:01 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-05-06 21:37:59 PDT
[JSC] LLIntPrototypeLoadAdaptiveStructureWatchpoint does not require Bag<>
Comment 1 Yusuke Suzuki 2019-05-06 21:45:03 PDT
Created attachment 369228 [details]
Patch
Comment 2 Robin Morisset 2019-05-06 22:13:09 PDT
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 3 Yusuke Suzuki 2019-05-06 22:15:09 PDT
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 4 Build Bot 2019-05-07 02:01:49 PDT
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
Comment 5 Build Bot 2019-05-07 02:01:51 PDT
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 6 Saam Barati 2019-05-07 09:57:30 PDT
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 7 Yusuke Suzuki 2019-05-07 20:48:57 PDT
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".
Comment 8 Yusuke Suzuki 2019-05-07 21:35:58 PDT
Committed r245050: <https://trac.webkit.org/changeset/245050>
Comment 9 Radar WebKit Bug Importer 2019-05-07 21:36:20 PDT
<rdar://problem/50569245>
Comment 10 Darin Adler 2019-05-08 12:56:13 PDT
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.
Comment 11 Saam Barati 2019-05-08 16:30:05 PDT
(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.
Comment 12 Yusuke Suzuki 2019-05-09 15:00:12 PDT
(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 :)
Comment 13 Yusuke Suzuki 2019-05-09 15:17:53 PDT
(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 :)