This is an archive of the discontinued LLVM Phabricator instance.

MS Compat: interpose vadefs.h to fix definitions of _crt_va_{start,end,arg} (PR21247)
ClosedPublic

Authored by hans on Oct 14 2014, 2:14 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

hans updated this revision to Diff 14889.Oct 14 2014, 2:14 PM
hans retitled this revision from to MS Compat: interpose vadefs.h to fix definitions of _crt_va_{start,end,arg} (PR21247).
hans updated this object.
hans edited the test plan for this revision. (Show Details)
hans added reviewers: rnk, majnemer.
hans added subscribers: Unknown Object (MLST), hansw.
rnk added inline comments.Oct 14 2014, 3:19 PM
lib/Headers/vadefs.h
24 ↗(On Diff #14889)

I'd wordsmith this to "Only include this if we're aiming for MSVC compatibility."

32 ↗(On Diff #14889)

Should we only do this if __has_include(<vadefs.h>) ?

34 ↗(On Diff #14889)

I think we should do "#ifndef _crt_va_start #undef _crt_va_start" to avoid warnings with -Wsystem-headers.

hans added inline comments.Oct 14 2014, 3:34 PM
lib/Headers/vadefs.h
24 ↗(On Diff #14889)

Done.

32 ↗(On Diff #14889)

I think we should always do it. If the user is including vadefs.h, she's expecting to get the system one, and we should just always try to forward to it.

34 ↗(On Diff #14889)

Done.

hans updated this revision to Diff 14899.Oct 14 2014, 3:34 PM

Addressing rnk's comments.

I tried this on a small program with -Wsystem-headers, and it was clean.

rnk accepted this revision.Oct 14 2014, 3:40 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Oct 14 2014, 3:40 PM
hans closed this revision.Oct 14 2014, 3:46 PM
hans updated this revision to Diff 14901.

Closed by commit rL219740 (authored by @hans).