...
Created attachment 291295 [details] patch
Comment on attachment 291295 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:62 > + bitwise_cast<HasOwnPropertyCache*>(cache)->clear(); I'll make this a static_cast
Attachment 291295 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:116: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 291295 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:133 > memset(this, 0, sizeof(Entry) * size); Could you just placement new instead?
Comment on attachment 291295 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:123 > + bitwise_cast<Entry*>(this)[i].Entry::~Entry(); Pointer casting can use reinterpret_cast.
(In reply to comment #5) > Comment on attachment 291295 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291295&action=review > > > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:123 > > + bitwise_cast<Entry*>(this)[i].Entry::~Entry(); > > Pointer casting can use reinterpret_cast. I prefer bitwise_cast for this still because it does the sizeof check. Which prevents e from making mistakes like this: unsigned bar = ...; Foo* ptr = bitwise_cast<Foo*>(bar);
Created attachment 291315 [details] patch for landing
Attachment 291315 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:118: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 291315 [details] patch for landing Clearing flags on attachment: 291315 Committed r207186: <http://trac.webkit.org/changeset/207186>
All reviewed patches have been landed. Closing bug.
This broke the build in several linux bots (GTK+, JSCOnly). Maybe something not supported by GCC 4.9? https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/2318/steps/compile-webkit/logs/stdio
(In reply to comment #11) > This broke the build in several linux bots (GTK+, JSCOnly). Maybe something > not supported by GCC 4.9? > > https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/ > builds/2318/steps/compile-webkit/logs/stdio I also noticed this error, adding Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool) constructor fixed the build for me. I'm goning to land it immediately.
Fix landed in http://trac.webkit.org/changeset/207213. (In reply to comment #11) > This broke the build in several linux bots (GTK+, JSCOnly). Maybe something > not supported by GCC 4.9? > > https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/ > builds/2318/steps/compile-webkit/logs/stdio But the question is why the GTK EWS was green? Do you have different compiler on the EWS and buildbots? It would be great to keep them in sync.
Additionally it was broken on Apple Windows and WinCairo bots too. Is it a compiler bug in MSVC and GCC? Is C++11/14/17 allows the compiler to generate Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool) constructor?
(In reply to comment #14) > Additionally it was broken on Apple Windows and WinCairo bots too. > > Is it a compiler bug in MSVC and GCC? Is C++11/14/17 allows the compiler to > generate Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool) > constructor? I found the proper answer for this problem. In C++11 Entry isn't an aggregate because of brace initializers of its members, but it is in C++14. http://stackoverflow.com/questions/35891233/initializing-a-struct-with-aggregate-initialization-and-member-initializers The problem here is that GCC 4.9 and VS 2015 doesn't support NSDMIs for aggregates.
(In reply to comment #13) > Fix landed in http://trac.webkit.org/changeset/207213. > > > (In reply to comment #11) > > This broke the build in several linux bots (GTK+, JSCOnly). Maybe something > > not supported by GCC 4.9? > > > > https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/ > > builds/2318/steps/compile-webkit/logs/stdio > > But the question is why the GTK EWS was green? Do you have different > compiler on the EWS and buildbots? Yes, unfortunately. > It would be great to keep them in sync. Agree
I believe the build fix added reference count churn: + Entry(RefPtr<UniquedStringImpl> impl, StructureID structureID, bool result) + : impl(impl) This is not the same thing as the automatically generated behavior, which can do a move if passed a RefPtr&&.
(In reply to comment #17) > I believe the build fix added reference count churn: > > + Entry(RefPtr<UniquedStringImpl> impl, StructureID structureID, bool > result) > + : impl(impl) > > This is not the same thing as the automatically generated behavior, which > can do a move if passed a RefPtr&&. Yeah I believe it should always be passed a RefPtr&&. This should be an easy fix. Does the `StructName { ... }` syntax call the constructor?
(In reply to comment #18) > Does the `StructName { ... }` syntax call the constructor? Yes.
(In reply to comment #19) > (In reply to comment #18) > > Does the `StructName { ... }` syntax call the constructor? > > Yes. Interesting. I had thought the "{ ... }" just individually constructs each field. Do you know what the rules are for it calling a constructor? Must there be a constructor that matches the types? Like StructName{ T1,...,TN } Calls constructor of type ConstructorTye(T1, ..., TN) ? (Ignoring type conversions, etc)
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > Does the `StructName { ... }` syntax call the constructor? > > > > Yes. > > Interesting. I had thought the "{ ... }" just individually constructs each > field. Only if there is no constructor. > Do you know what the rules are for it calling a constructor? An oversimplification: direct initialization of the data members with that syntax works only if there are no constructors. For the full story, C++ reference material recommended. I normally use cppreference.com to look things up. Must there be a > constructor that matches the types? Like > StructName{ T1,...,TN } > Calls constructor of type > ConstructorTye(T1, ..., TN) > ? > (Ignoring type conversions, etc) Yes. In fact, the ClassName { } syntax does not allow many type conversions that the ClassName ( ) syntax performs, which is another benefit of using it.
I've opened this bug to go back to move semantics: https://bugs.webkit.org/show_bug.cgi?id=163490
(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #19) > > > (In reply to comment #18) > > > > Does the `StructName { ... }` syntax call the constructor? > > > > > > Yes. > > > > Interesting. I had thought the "{ ... }" just individually constructs each > > field. > > Only if there is no constructor. > > > Do you know what the rules are for it calling a constructor? > > An oversimplification: direct initialization of the data members with that > syntax works only if there are no constructors. For the full story, C++ > reference material recommended. I normally use cppreference.com to look > things up. Interesting. I'll read about this. It's surprising to me because I had just assumed the syntax was more or less shorthand for initializing each field. That was a bad assumption on my part. > > Must there be a > > constructor that matches the types? Like > > StructName{ T1,...,TN } > > Calls constructor of type > > ConstructorTye(T1, ..., TN) > > ? > > (Ignoring type conversions, etc) > > Yes. In fact, the ClassName { } syntax does not allow many type conversions > that the ClassName ( ) syntax performs, which is another benefit of using it.