[JSC] Use node index as DFG::MinifiedID
Created attachment 368011 [details] Patch
<rdar://problem/50118963>
Attachment 368011 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:42: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:43: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:44: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 368013 [details] Patch
Attachment 368013 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:41: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:42: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:43: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 368013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368013&action=review > Source/JavaScriptCore/dfg/DFGMinifiedID.h:67 > + uintptr_t bits() const { return m_index; } What is this used for? Why still return uintptr?
Comment on attachment 368013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368013&action=review >> Source/JavaScriptCore/dfg/DFGMinifiedID.h:67 >> + uintptr_t bits() const { return m_index; } > > What is this used for? Why still return uintptr? Nice catch. It is originally introduced to make MinifiedID uintptr_t or some integer types to be included one member of union. But now, C++11 allows us to put this class directly in the union. So, we do not need to have this. I'll remove it.
Comment on attachment 368013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368013&action=review >>> Source/JavaScriptCore/dfg/DFGMinifiedID.h:67 >>> + uintptr_t bits() const { return m_index; } >> >> What is this used for? Why still return uintptr? > > Nice catch. It is originally introduced to make MinifiedID uintptr_t or some integer types to be included one member of union. But now, C++11 allows us to put this class directly in the union. So, we do not need to have this. > I'll remove it. Ah, no. Having non-trivial constructor makes putting it in union difficult. I'll just continue using this bits / fromBits mechanism in this patch.
Comment on attachment 368013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368013&action=review >>>> Source/JavaScriptCore/dfg/DFGMinifiedID.h:67 >>>> + uintptr_t bits() const { return m_index; } >>> >>> What is this used for? Why still return uintptr? >> >> Nice catch. It is originally introduced to make MinifiedID uintptr_t or some integer types to be included one member of union. But now, C++11 allows us to put this class directly in the union. So, we do not need to have this. >> I'll remove it. > > Ah, no. Having non-trivial constructor makes putting it in union difficult. I'll just continue using this bits / fromBits mechanism in this patch. And I fixed this, using unsigned instead of uintptr_t.
Created attachment 368058 [details] Patch
Attachment 368058 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:42: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGMinifiedID.h:43: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 368058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368058&action=review Lgtm overall > Source/JavaScriptCore/dfg/DFGMinifiedIDInlines.h:35 > +inline MinifiedID::MinifiedID(Node* node) What is the point of putting this function in its own file? Considering how trivial it is, I doubt it has any impact on compile time.
Comment on attachment 368058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368058&action=review >> Source/JavaScriptCore/dfg/DFGMinifiedIDInlines.h:35 >> +inline MinifiedID::MinifiedID(Node* node) > > What is the point of putting this function in its own file? > Considering how trivial it is, I doubt it has any impact on compile time. This is because we do not put DFGNode.h as Private header (exposed to the rest of the WebKit), while DFGMinifiedID.h is included by ValueRecovery and it is included by some headers that is annotated as Private exposed. We do not want to make DFGNode.h and dependent headers Private-exposed. To avoid this, we have two choices, 1. Put MinifiedID::MinifiedID implementation in DFGMinifiedID.cpp 2. Add DFGMinifiedIDInlines.h Here I took (2) option because the implementation of MinifiedID::MinifiedID is small.
(In reply to Yusuke Suzuki from comment #13) > Comment on attachment 368058 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368058&action=review > > >> Source/JavaScriptCore/dfg/DFGMinifiedIDInlines.h:35 > >> +inline MinifiedID::MinifiedID(Node* node) > > > > What is the point of putting this function in its own file? > > Considering how trivial it is, I doubt it has any impact on compile time. > > This is because we do not put DFGNode.h as Private header (exposed to the > rest of the WebKit), while DFGMinifiedID.h is included by ValueRecovery and > it is included by some headers that is annotated as Private exposed. > We do not want to make DFGNode.h and dependent headers Private-exposed. To > avoid this, we have two choices, > > 1. Put MinifiedID::MinifiedID implementation in DFGMinifiedID.cpp > 2. Add DFGMinifiedIDInlines.h > > Here I took (2) option because the implementation of MinifiedID::MinifiedID > is small. That makes sense, thanks for the answer.
Comment on attachment 368058 [details] Patch Thanks Saam and Robin!
Comment on attachment 368058 [details] Patch Rejecting attachment 368058 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 368058, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=368058&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=197186&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Updating working directory Processing patch 368058 from bug 197186. Fetching: https://bugs.webkit.org/attachment.cgi?id=368058 Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Committing to http://svn.webkit.org/repository/webkit/trunk ... Can't create session: Unable to connect to a repository at URL 'http://svn.webkit.org/repository/webkit/trunk': Error running context: The server unexpectedly closed the connection. at /Applications/Xcode.app/Contents/Developer/usr/libexec/git-core/git-svn line 1033. Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Committing to http://svn.webkit.org/repository/webkit/trunk ... Can't create session: Unable to connect to a repository at URL 'http://svn.webkit.org/repository/webkit/trunk': Error running context: The server unexpectedly closed the connection. at /Applications/Xcode.app/Contents/Developer/usr/libexec/git-core/git-svn line 1033. Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource From https://git.webkit.org/git/WebKit 165bf4efab5..908452fed98 master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 244570 = 165bf4efab5cfcf64ee0183401b1003be50e82f5 r244571 = fd4abbc1ad1f0f8c99119c1d366e7c04bf46d69b r244572 = 908452fed98f80ee058f1b2535fc24e5a6efa3da Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: https://webkit-queues.webkit.org/results/11976964
Committed r244578: <https://trac.webkit.org/changeset/244578>