Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(985)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 8 months ago by Wladimir Palant
Modified:
2 years, 7 months ago
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
2 years, 8 months ago (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 ...
2 years, 8 months ago (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 ...
2 years, 7 months ago (2017-03-22 08:52:12 UTC) #3
Vasily Kuznetsov
2 years, 7 months ago (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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5