Page MenuHomePhabricator

[libcxx] [test] Avoid conflicting definitions of _CRT_SECURE_NO_WARNINGS
ClosedPublic

Authored by mstorsjo on Fri, Oct 16, 1:06 PM.

Details

Summary

This is defined both by libcxx/utils/libcxx/test/config.py (for any windows target) and msvc_stdlib_force_include.h (when testing specifically the MSVC C++ library).

The command line define (-D_CRT_SECURE_NO_WARNINGS) defines it to the value 1; change the header define to match that.

Keeping both instances, to keep the fix for cases when not building in cases that don't use config.py.

Also remove a comment about whether this can be removed; it can't at least be removed altogether - doing that breaks a number of tests that otherwise succeed.

Diff Detail

Event Timeline

mstorsjo created this revision.Fri, Oct 16, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Oct 16, 1:06 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Fri, Oct 16, 1:06 PM
ldionne requested changes to this revision.Fri, Oct 16, 1:13 PM
ldionne added a subscriber: ldionne.

Instead, do you think it would make sense to only keep this #define? I'm trying to remove logic in config.py.

This revision now requires changes to proceed.Fri, Oct 16, 1:13 PM

Instead, do you think it would make sense to only keep this #define? I'm trying to remove logic in config.py.

The difference is that this header is only included when testing the MS STL, while the defines in config.py are added also when testing libc++ itself. These defines are used to silence things in the MSVC C standard library, which would be used (presumably by the test support code) even if testing libc++ itself.

I'm rerunning the whole testsuite to see if this particular define can be removed from config.py as well (there's a fixme comment asking whether it can be removed).

For the filesystem stuff, I'd like to add _CRT_NONSTDC_NO_WARNINGS similarly, which avoids needing to do anything at all regarding the functions chdir, fileno and getcwd in the support header.

I'm rerunning the whole testsuite to see if this particular define can be removed from config.py as well (there's a fixme comment asking whether it can be removed).

No, removing the define altogether breaks 45 tests that succeeded before.

mstorsjo updated this revision to Diff 298752.Fri, Oct 16, 2:08 PM

Also remove a fixme comment regarding this define - it can't be removed from config.py.

CaseyCarter added inline comments.
libcxx/test/support/msvc_stdlib_force_include.h
17

Instead of striking this, could we instead define it conditionally to 1:

#ifndef _CRT_SECURE_NO_WARNINGS
#define _CRT_SECURE_NO_WARNINGS 1
#endif // _CRT_SECURE_NO_WARNINGS

to preserve use cases that don't involve config.py?

mstorsjo added inline comments.Fri, Oct 16, 10:21 PM
libcxx/test/support/msvc_stdlib_force_include.h
17

Sure, that's fine by me as well. And no new ifdef is needed if we just add the 1 to the definition.

CaseyCarter added inline comments.Sat, Oct 17, 5:07 PM
libcxx/test/support/msvc_stdlib_force_include.h
17

Yes, thanks - I forget that redefinition is allowed when the replacement lists are identical.

mstorsjo updated this revision to Diff 298895.Sun, Oct 18, 12:30 PM
mstorsjo retitled this revision from [libcxx] [test] Remove a duplicate definition of _CRT_SECURE_NO_WARNINGS to [libcxx] [test] Avoid conflicting definitions of _CRT_SECURE_NO_WARNINGS.
mstorsjo edited the summary of this revision. (Show Details)

Kept both instances of the define but adjusted them to match.

Making the definitions non-conflicting looks good to me, thanks.

@ldionne Any more comments on this?

ldionne accepted this revision.Tue, Oct 20, 9:14 AM
This revision is now accepted and ready to land.Tue, Oct 20, 9:14 AM