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

Issue 1133423003: Work around a clang/win bug, add a .h file to gyp/gn files. (Closed)

Created:
5 years, 7 months ago by Nico
Modified:
5 years, 7 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Work around a clang/win bug, add a .h file to gyp/gn files. clang can't handle dllexported classes with only member initializers yet (see crbug and http://llvm.org/PR23542). As a workaround, don't dllexport this class. This has the effect of emitting the class's constructor into every translation unit that uses it instead of just every translation unit of the media component, but in a way that seems nicer anyways since there's no .cc file for this header and so currently this relies on some translation unit in media including this header by lucky accident (else the constructor wouldn't be defined anywhere.) Once the clang bug is fixed we can consider adding this back; in the meantime this seems like a tiny price to pay to get the bots back green. Also add the .h file to the relevant gyp/gn files, else the analyze step will make the trybots think that nothing changed when this .h file is touched. BUG=488634, 467779 TBR=sandersd Committed: https://crrev.com/8a0f0753b1ece73c5636b263f4c4a13f75fcb2cd Cr-Commit-Position: refs/heads/master@{#330299}

Patch Set 1 #

Patch Set 2 : buildfiles #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -1 line) Patch
M media/base/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M media/base/cdm_config.h View 1 chunk +1 line, -1 line 0 comments Download
M media/media.gyp View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 10 (2 generated)
Nico
5 years, 7 months ago (2015-05-17 02:21:01 UTC) #2
Nico
I'll tbr this to get the bots back green.
5 years, 7 months ago (2015-05-17 03:26:58 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133423003/20001
5 years, 7 months ago (2015-05-17 03:27:19 UTC) #5
Nico
(But please do give it a look when you get the chance!)
5 years, 7 months ago (2015-05-17 03:27:28 UTC) #6
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 7 months ago (2015-05-17 03:31:02 UTC) #7
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/8a0f0753b1ece73c5636b263f4c4a13f75fcb2cd Cr-Commit-Position: refs/heads/master@{#330299}
5 years, 7 months ago (2015-05-18 11:32:24 UTC) #8
hans
lgtm Thanks for fixing this. I'll try to get to the Clang bug this week.
5 years, 7 months ago (2015-05-18 16:16:44 UTC) #9
sandersd (OOO until July 31)
5 years, 7 months ago (2015-05-18 18:01:49 UTC) #10
Message was sent while issue was closed.
lgtm

Powered by Google App Engine
This is Rietveld 408576698