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

Issue 10386108: Change the way _EXPORT macros look. (Closed)

Created:
8 years, 7 months ago by Nico
Modified:
8 years, 7 months ago
CC:
chromium-reviews, michaeln, cbentzel+watch_chromium.org, sadrul, ben+watch_chromium.org, Ian Vollick, erikwright (departed), jonathan.backer, jam, apatrick_chromium, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, dyu1, brettw-cc_chromium.org, piman+watch_chromium.org, Ilya Sherman, dhollowa+watch_chromium.org, jshin+watch_chromium.org
Visibility:
Public.

Description

Change the way _EXPORT macros look. With the current setup, if you have a header file my_class.h class BASE_EXPORT MyClass { public: void MyInlineMethod() { /* do stuff, inline */ } }; then every cc file that includes my_class.h will have a public symbol for MyInlineMethod (because inline methods need to be emitted to every translation unit, and the linker sorts them out). With the components build, the linker can't decide to drop these inline methods, so every .so that uses this header file will have the same public symbol. With this proposed change, the symbol will only be visible in the target the header file belongs to, and it will be hidden in all other components. That's cleaner, and it also prevents accident hidden dependencies (say target A depends on B, and B depends on C. A accidentally uses an inline function from a class in C. With this change, that would result in a linker error, and an explicit dependency from A on C would have to be added). Also add a missing CHROMEOS_IMPLEMENTATION define which went unnoticed until now. BUG=90078 TEST=Things still build. TBR=ben, tony, viettrungluu, thestig, agl, willchan Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137130

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : cros #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+124 lines, -0 lines) Patch
M ash/ash_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M base/base_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M base/i18n/base_i18n_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M chromeos/chromeos.gyp View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M chromeos/chromeos_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M content/common/content_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M crypto/crypto_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_c_lib_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M gpu/command_buffer/client/gles2_impl_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M gpu/command_buffer/common/gles2_utils_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M gpu/gpu_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ipc/ipc_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M media/base/media_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M net/base/net_export.h View 1 chunk +5 lines, -0 lines 0 comments Download
M ppapi/proxy/ppapi_proxy_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ppapi/shared_impl/ppapi_shared_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ppapi/thunk/ppapi_thunk_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M printing/printing_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M sql/sql_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/aura/aura_export.h View 1 chunk +4 lines, -0 lines 2 comments Download
M ui/base/ui_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/compositor/compositor_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/gl/gl_export.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M ui/oak/oak_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/surface/surface_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/views/views_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M webkit/appcache/appcache_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M webkit/blob/blob_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M webkit/forms/webkit_forms_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M webkit/glue/webkit_glue_export.h View 1 chunk +4 lines, -0 lines 0 comments Download
M webkit/plugins/webkit_plugins_export.h View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
Nico
Mostly for evan.
8 years, 7 months ago (2012-05-14 20:23:13 UTC) #1
Evan Martin
LGTM if it doesn't break anything. On Mon, May 14, 2012 at 1:23 PM, <thakis@chromium.org> ...
8 years, 7 months ago (2012-05-15 04:27:44 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/1
8 years, 7 months ago (2012-05-15 04:45:56 UTC) #3
commit-bot: I haz the power
Can't apply patch for file ui/gfx/gl/gl_export.h. While running patch -p1 --forward --force; A ui/gfx/gl patching ...
8 years, 7 months ago (2012-05-15 04:46:13 UTC) #4
Nico
rebase
8 years, 7 months ago (2012-05-15 04:51:43 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/4002
8 years, 7 months ago (2012-05-15 04:52:47 UTC) #6
commit-bot: I haz the power
Presubmit check for 10386108-4002 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 7 months ago (2012-05-15 04:53:03 UTC) #7
Nico
ben, tony, viettrungluu, thestig, agl, willchan: OWNERS tbr
8 years, 7 months ago (2012-05-15 05:00:18 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/4002
8 years, 7 months ago (2012-05-15 05:02:17 UTC) #9
commit-bot: I haz the power
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is ...
8 years, 7 months ago (2012-05-15 06:06:32 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/4002
8 years, 7 months ago (2012-05-15 06:08:03 UTC) #11
commit-bot: I haz the power
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is ...
8 years, 7 months ago (2012-05-15 07:11:56 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/4002
8 years, 7 months ago (2012-05-15 13:01:48 UTC) #13
commit-bot: I haz the power
Try job failure for 10386108-4002 (retry) on linux_chromeos for step "compile" (clobber build). It's a ...
8 years, 7 months ago (2012-05-15 13:39:50 UTC) #14
Nico
cros
8 years, 7 months ago (2012-05-15 14:01:23 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/1034
8 years, 7 months ago (2012-05-15 14:02:37 UTC) #16
Ben Goodger (Google)
lgtm
8 years, 7 months ago (2012-05-15 16:09:40 UTC) #17
tfarina
https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h File ui/aura/aura_export.h (right): https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h#newcode22 ui/aura/aura_export.h:22: #if defined(AURA_IMPLEMENTATION) nit: should we do the same to ...
8 years, 7 months ago (2012-05-15 18:07:23 UTC) #18
Nico
https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h File ui/aura/aura_export.h (right): https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h#newcode22 ui/aura/aura_export.h:22: #if defined(AURA_IMPLEMENTATION) On 2012/05/15 18:07:24, tfarina wrote: > nit: ...
8 years, 7 months ago (2012-05-15 18:15:42 UTC) #19
tfarina
On 2012/05/15 18:15:42, Nico wrote: > https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h > File ui/aura/aura_export.h (right): > > https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h#newcode22 > ...
8 years, 7 months ago (2012-05-15 18:19:20 UTC) #20
tfarina
8 years, 7 months ago (2012-05-15 18:20:36 UTC) #21
On Tue, May 15, 2012 at 3:19 PM,  <tfarina@chromium.org> wrote:
>> Yes. I tried to add it to all _export.h files.
>
> Should I send a patch for review?
>
Ah, I see that you are already fixing it.


-- 
Thiago

Powered by Google App Engine
This is Rietveld 408576698