Why do we have NON_EXPORTED_BASE instead of ignoring VS warning C4275?

[John Abd-El-Malek]

It seems that every time we run into this warning which we treat as an error, we just add the macro. This makes readability harder and gives surprise build breaks to people not working on Windows.

Is anyone against making our gyp files ignore this warning and removing all instances of this macro?

[Nico Weber]

From http://codereview.chromium.org/6969077 (which added this):

"http://msdn.microsoft.com/en-us/library/3tdb471s(VS.80).aspx

It might also be good to mention that you need to be very careful if you have
static data in the base class, and to see the article for more."

So just blindly adding this macro is apparently dangerous. But
requiring the macro might not have helped to raise awareness if
everybody just blindly adds it as you say.

(I don't have an opinion; just adding background.)

Nico

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

[John Abd-El-Malek]

On Wed, Feb 8, 2012 at 10:51 AM, Nico Weber <tha...@chromium.org> wrote:

From http://codereview.chromium.org/6969077 (which added this):

"http://msdn.microsoft.com/en-us/library/3tdb471s(VS.80).aspx

It might also be good to mention that you need to be very careful if you have
static data in the base class, and to see the article for more." 

So just blindly adding this macro is apparently dangerous. But
requiring the macro might not have helped to raise awareness if
everybody just blindly adds it as you say.

It seems the issue with static data will exist across all compilers when using the components build. So why would we warn just on VS?

[Dirk Pranke]

On Wed, Feb 8, 2012 at 11:32 AM, John Abd-El-Malek
<jabde...@google.com> wrote:
>
>
> On Wed, Feb 8, 2012 at 10:51 AM, Nico Weber <tha...@chromium.org> wrote:
>>
>> From http://codereview.chromium.org/6969077 (which added this):
>>
>> "http://msdn.microsoft.com/en-us/library/3tdb471s(VS.80).aspx
>>
>> It might also be good to mention that you need to be very careful if you
>> have
>> static data in the base class, and to see the article for more."
>>
>>
>> So just blindly adding this macro is apparently dangerous. But
>> requiring the macro might not have helped to raise awareness if
>> everybody just blindly adds it as you say.
>
>
> It seems the issue with static data will exist across all compilers when
> using the components build. So why would we warn just on VS?
>

AFAIK, VS is the only compiler that generates this warning; if they
others can, they probably should be.

As the article Nico references points out, this macro shouldn't be
added blindly, and it's the coder's (and reviewer's) job to know when
this is safe or not. I would not recommend turning the warning off.

-- Dirk

[Dirk Pranke]

Sorry, I should have also noted that there were many times when I was
working on component builds where I ran across this warning, and it
told me that the correct thing to do was to export the base class
(which is the alternative). So, we don't all always blindly add it :).

> -- Dirk

[John Abd-El-Malek]

I see. I wasn't aware of the static issue before this thread. I've only been encountering this macro when writing/reviewing changes for the content api where all the base classes are interfaces.

Since it seems this is dangerous to ignore in some cases, I'll add the warning about statics in the comment above the macro.

 

> -- Dirk

[Nico Weber]

A bit over 5 years later, I'm about to disable this warning and remove NON_EXPORTED_BASE: https://chromium-review.googlesource.com/c/603773

Please shout if you see an issue with that.