Bug 197645 – [JSC] LLIntPrototypeLoadAdaptiveStructureWatchpoint does not require Bag<>

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] LLIntPrototypeLoadAdaptiveStructureWatchpoint does not require Bag<>

[Yusuke Suzuki]

Created attachment 369228 [details]
Patch

[Robin Morisset]

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

[Yusuke Suzuki]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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?

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Committed r245050: <https://trac.webkit.org/changeset/245050>

[Radar WebKit Bug Importer]

<rdar://problem/50569245>

[Darin Adler]

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.

[Saam Barati]

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

[Yusuke Suzuki]

(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 :)

[Yusuke Suzuki]

(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 :)