Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 1553403002: re2: Drop dynamic_annotations dependency and remove compiler warning flags (Closed)

Created:
4 years, 11 months ago by battre
Modified:
4 years, 11 months ago
Reviewers:
tfarina, Nico
CC:
chromium-reviews, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

re2: Drop dynamic_annotations dependency and remove compiler warning flags This CL removes some build file specifications that are not needed anymore. BUG=568119 Committed: https://crrev.com/c1aa567aeb8343b60d832973a024678dcb8a86c6 Cr-Commit-Position: refs/heads/master@{#367539}

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+0 lines, -17 lines) Patch
M third_party/re2/BUILD.gn View 1 chunk +0 lines, -11 lines 0 comments Download
M third_party/re2/re2.gyp View 2 chunks +0 lines, -6 lines 4 comments Download

Messages

Total messages: 14 (7 generated)
battre
Hi Nico, could you please review this CL? Thanks, Dominic
4 years, 11 months ago (2016-01-05 13:28:59 UTC) #2
Nico
lgtm, thanks. I prefixed the subject line of your cl description with "re2:".
4 years, 11 months ago (2016-01-05 15:55:14 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1553403002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1553403002/1
4 years, 11 months ago (2016-01-05 15:56:02 UTC) #7
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 11 months ago (2016-01-05 16:00:27 UTC) #9
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/c1aa567aeb8343b60d832973a024678dcb8a86c6 Cr-Commit-Position: refs/heads/master@{#367539}
4 years, 11 months ago (2016-01-05 16:01:23 UTC) #11
tfarina
lgtm, thanks! https://codereview.chromium.org/1553403002/diff/1/third_party/re2/re2.gyp File third_party/re2/re2.gyp (right): https://codereview.chromium.org/1553403002/diff/1/third_party/re2/re2.gyp#newcode7 third_party/re2/re2.gyp:7: 'build_for_tool%': '', what is this for? Can ...
4 years, 11 months ago (2016-01-05 16:50:21 UTC) #13
battre
4 years, 11 months ago (2016-01-05 16:56:08 UTC) #14
Message was sent while issue was closed.
https://codereview.chromium.org/1553403002/diff/1/third_party/re2/re2.gyp
File third_party/re2/re2.gyp (right):

https://codereview.chromium.org/1553403002/diff/1/third_party/re2/re2.gyp#new...
third_party/re2/re2.gyp:7: 'build_for_tool%': '',
On 2016/01/05 16:50:21, tfarina wrote:
> what is this for? Can we get rid of it?

This is needed. It declares build_for_tool as a variable that is empty by
default but can be overridden.

Without this line, 'build_for_tool=="drmemory"' would fail if you don't set the
variable.

https://codereview.chromium.org/1553403002/diff/1/third_party/re2/re2.gyp#new...
third_party/re2/re2.gyp:13: 'include_dirs': [
On 2016/01/05 16:50:21, tfarina wrote:
> is this and direct_dependent_settings really needed?

hm, no idea.

Powered by Google App Engine
This is Rietveld 408576698