Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29390663: Issue 5020 - Part 3: Add command line options to compile script (Closed)

Created:
March 21, 2017, 11:22 a.m. by Wladimir Palant
Modified:
March 27, 2017, 7:59 a.m.
Reviewers:
Vasily Kuznetsov
CC:
hub
Base URL:
https://hg.adblockplus.org/adblockpluscore
Visibility:
Public.

Description

Issue 5020 - Part 3: Add command line options to compile script

Patch Set 1 #

Total comments: 2

Patch Set 2 : Improved the way compiler parameters are passed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -6 lines) Patch
M compile View 1 3 chunks +31 lines, -6 lines 0 comments Download

Messages

Total messages: 4
Wladimir Palant
March 21, 2017, 11:22 a.m. (2017-03-21 11:22:53 UTC) #1
Vasily Kuznetsov
LGTM I have one small concern (see below) but I think it's not serious enough ...
March 21, 2017, 6:38 p.m. (2017-03-21 18:38:19 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29390663/diff/29390664/compile File compile (right): https://codereview.adblockplus.org/29390663/diff/29390664/compile#newcode106 compile:106: ADDITIONAL_PARAMS.append('-g1') On 2017/03/21 18:38:19, Vasily Kuznetsov wrote: > Isn't ...
March 22, 2017, 8:52 a.m. (2017-03-22 08:52:12 UTC) #3
Vasily Kuznetsov
March 22, 2017, 9:58 a.m. (2017-03-22 09:58:10 UTC) #4
On 2017/03/22 08:52:12, Wladimir Palant wrote:
> https://codereview.adblockplus.org/29390663/diff/29390664/compile
> File compile (right):
> 
> https://codereview.adblockplus.org/29390663/diff/29390664/compile#newcode106
> compile:106: ADDITIONAL_PARAMS.append('-g1')
> On 2017/03/21 18:38:19, Vasily Kuznetsov wrote:
> > Isn't it a bit misleading that ADDITIONAL_PARAMETERS is named like a
constant
> > but we're modifying it?
> 
> I have been wondering the same thing. Yes, let's do it properly.

Yeah, this is nicer. LGTM

Powered by Google App Engine
This is Rietveld