Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537973004:
[regexp] Move source and species getter to TF (Closed)
Description[regexp] Move source and species getter to TF
BUG=v8:5339
Committed: https://crrev.com/52016b65e5e83cc85e8fc4e73f7b2bfcb8217e58
Cr-Commit-Position: refs/heads/master@{#41432}
Patch Set 1 #Patch Set 2 : Simplify instance type check #
Total comments: 14
Patch Set 3 : Address comments #
Messages
Total messages: 22 (14 generated)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jgruber@chromium.org changed reviewers: + cbruni@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:701: void Builtins::Generate_RegExpPrototypeSourceGetter(CodeAssemblerState* state) { Could you migrate this to the new and much cleaner TF builtins (See TF_BUILTIN in builtins-number.cc)? https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:711: &if_isnotjsregexp); If you want to go the extra mile, you'd probably add something like BranchIfIsJSRegexp on the custom RegExpBuiltinsAssembler subclass. https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:726: a.LoadObjectField(regexp_fun, JSFunction::kPrototypeOrInitialMapOffset); this sequence of loading the default regexp's map would also make a nice helper :) https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:736: Node* const counter_smi = a.SmiConstant(Smi::FromInt(counter)); nit: you can directly pass in the int to SmiConstant. https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:753: a.Return(a.UndefinedConstant()); // Never reached. Could you do a TailCallRuntime here? https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:797: CodeAssemblerState* state) { ditto: please port to new builtins style
ishell@chromium.org changed reviewers: + ishell@chromium.org
https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:701: void Builtins::Generate_RegExpPrototypeSourceGetter(CodeAssemblerState* state) { On 2016/12/01 12:24:47, Camillo Bruni wrote: > Could you migrate this to the new and much cleaner TF builtins (See TF_BUILTIN > in builtins-number.cc)? This requires a separate CL.
https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:701: void Builtins::Generate_RegExpPrototypeSourceGetter(CodeAssemblerState* state) { On 2016/12/01 at 12:29:53, Igor Sheludko wrote: > On 2016/12/01 12:24:47, Camillo Bruni wrote: > > Could you migrate this to the new and much cleaner TF builtins (See TF_BUILTIN > > in builtins-number.cc)? > > This requires a separate CL. probably cleaner that way.
Thanks for the review, I'll migrate to the new TF builtin style & extract helper functions as suggested in another CL. https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:701: void Builtins::Generate_RegExpPrototypeSourceGetter(CodeAssemblerState* state) { On 2016/12/01 12:24:47, Camillo Bruni wrote: > Could you migrate this to the new and much cleaner TF builtins (See TF_BUILTIN > in builtins-number.cc)? Will do in a follow-up. Much cleaner indeed :) https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:711: &if_isnotjsregexp); On 2016/12/01 12:24:47, Camillo Bruni wrote: > If you want to go the extra mile, you'd probably add something like > BranchIfIsJSRegexp on the custom RegExpBuiltinsAssembler subclass. Acknowledged. https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:726: a.LoadObjectField(regexp_fun, JSFunction::kPrototypeOrInitialMapOffset); On 2016/12/01 12:24:47, Camillo Bruni wrote: > this sequence of loading the default regexp's map would also make a nice helper > :) Acknowledged. https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:736: Node* const counter_smi = a.SmiConstant(Smi::FromInt(counter)); On 2016/12/01 12:24:47, Camillo Bruni wrote: > nit: you can directly pass in the int to SmiConstant. Done. https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:753: a.Return(a.UndefinedConstant()); // Never reached. On 2016/12/01 12:24:47, Camillo Bruni wrote: > Could you do a TailCallRuntime here? Done. https://codereview.chromium.org/2537973004/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp.cc:797: CodeAssemblerState* state) { On 2016/12/01 12:24:47, Camillo Bruni wrote: > ditto: please port to new builtins style Acknowledged.
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2537973004/#ps40001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480601928038350,
"parent_rev": "2d7047c0c6c28f5a02c149c73a62fed77f469c98", "commit_rev":
"1bea160745dad83c5113c9f2d2b518af6c732178"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [regexp] Move source and species getter to TF BUG=v8:5339 ========== to ========== [regexp] Move source and species getter to TF BUG=v8:5339 Committed: https://crrev.com/52016b65e5e83cc85e8fc4e73f7b2bfcb8217e58 Cr-Commit-Position: refs/heads/master@{#41432} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/52016b65e5e83cc85e8fc4e73f7b2bfcb8217e58 Cr-Commit-Position: refs/heads/master@{#41432} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
