Remove unnecessary RewriteNonPatternArguments

Remove unnecessary RewriteNonPatternArguments

ParseArguments already does the rewriting.

R=rossberg@chromium.org
BUG=

Committed: https://crrev.com/14f4f8fc021912b9b220fd2dd32a95074ecba96f
Cr-Commit-Position: refs/heads/master@{#33441}

[rossberg, 2016-01-21 14:16:38]

https://codereview.chromium.org/1617733002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1617733002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4898: ValidateExpression(&classifier, CHECK_OK);
Hm, isn't this redundant as well? If the thing has already been rewritten then
it should also have been validated already. What case would this check capture?

[nickie, 2016-01-21 14:29:26]

https://codereview.chromium.org/1617733002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1617733002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4898: ValidateExpression(&classifier, CHECK_OK);
On 2016/01/21 14:16:37, rossberg wrote:
> Hm, isn't this redundant as well? If the thing has already been rewritten then
> it should also have been validated already. What case would this check
capture?

Honestly, I don't know.  This line was replaced with the call to
RewriteNonPatternArguments here:

https://codereview.chromium.org/1567603005/diff/120001/src/parsing/parser.cc#...

The ParseArguments function makes sure that each argument (if any exist) is
rewritten as a non-pattern.  This practically invokes ValidateExpression for
each argument, in order.  I'm not sure, however, if this is all you need here.

[rossberg, 2016-01-21 14:46:00]

https://codereview.chromium.org/1617733002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1617733002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4898: ValidateExpression(&classifier, CHECK_OK);
On 2016/01/21 14:29:25, nickie wrote:
> On 2016/01/21 14:16:37, rossberg wrote:
> > Hm, isn't this redundant as well? If the thing has already been rewritten
then
> > it should also have been validated already. What case would this check
> capture?
> 
> Honestly, I don't know.  This line was replaced with the call to
> RewriteNonPatternArguments here:
> 
>
https://codereview.chromium.org/1567603005/diff/120001/src/parsing/parser.cc#...
> 
> The ParseArguments function makes sure that each argument (if any exist) is
> rewritten as a non-pattern.  This practically invokes ValidateExpression for
> each argument, in order.  I'm not sure, however, if this is all you need here.

Let's remove it then it see. Having a dangling Validate without Rewrite seems
fishy. If it's actually necessary, I'd think we have a bug elsewhere.

[nickie, 2016-01-21 15:02:53]

https://codereview.chromium.org/1617733002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1617733002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4898: ValidateExpression(&classifier, CHECK_OK);
On 2016/01/21 14:46:00, rossberg wrote:
> On 2016/01/21 14:29:25, nickie wrote:
> > On 2016/01/21 14:16:37, rossberg wrote:
> > > Hm, isn't this redundant as well? If the thing has already been rewritten
> then
> > > it should also have been validated already. What case would this check
> > capture?
> > 
> > Honestly, I don't know.  This line was replaced with the call to
> > RewriteNonPatternArguments here:
> > 
> >
>
https://codereview.chromium.org/1567603005/diff/120001/src/parsing/parser.cc#...
> > 
> > The ParseArguments function makes sure that each argument (if any exist) is
> > rewritten as a non-pattern.  This practically invokes ValidateExpression for
> > each argument, in order.  I'm not sure, however, if this is all you need
here.
> 
> Let's remove it then it see. Having a dangling Validate without Rewrite seems
> fishy. If it's actually necessary, I'd think we have a bug elsewhere.

Acknowledged.

[nickie, 2016-01-21 15:18:12]

Patchset #2 (id:20001) has been deleted

[rossberg, 2016-01-21 15:36:51]

lgtm

[nickie, 2016-01-21 15:37:36]

The CQ bit was checked by nikolaos@chromium.org

[commit-bot: I haz the power, 2016-01-21 15:37:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1617733002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1617733002/40001

[commit-bot: I haz the power, 2016-01-21 15:59:33]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-01-21 16:00:18]

Description was changed from

==========
Remove unnecessary RewriteNonPatternArguments

ParseArguments already does the rewriting.

R=rossberg@chromium.org
BUG=
==========

to

==========
Remove unnecessary RewriteNonPatternArguments

ParseArguments already does the rewriting.

R=rossberg@chromium.org
BUG=

Committed: https://crrev.com/14f4f8fc021912b9b220fd2dd32a95074ecba96f
Cr-Commit-Position: refs/heads/master@{#33441}
==========

[commit-bot: I haz the power, 2016-01-21 16:00:19]

Patchset 2 (id:??) landed as
https://crrev.com/14f4f8fc021912b9b220fd2dd32a95074ecba96f
Cr-Commit-Position: refs/heads/master@{#33441}