This is an archive of the discontinued LLVM Phabricator instance.

PR26648: "inline" shouldn't be recognized as a C keyword in MSVC 2013 compatibility mode
AbandonedPublic

Authored by andreybokhanko on Feb 17 2016, 2:44 AM.

Details

Summary

See PR26648 for full description of the problem.

Here I added a new keyword flag (KEYNOMS18_C) for keywords that shouldn't be enabled in MSVC 2013 mode for C programs and set it for "inline".

Andrey

Diff Detail

Event Timeline

andreybokhanko retitled this revision from to PR26648: "inline" shouldn't be recognized as a C keyword in MSVC 2013 compatibility mode.
andreybokhanko updated this object.
andreybokhanko added reviewers: rnk, majnemer, thakis.
andreybokhanko added a subscriber: cfe-commits.
majnemer edited edge metadata.Feb 17 2016, 8:23 AM

Why not just stick clang in C90 mode when targeting C if the -fms-compatibility-version is 18?

We have similar code for the C++ mode in https://github.com/llvm-mirror/clang/blob/master/lib/Driver/Tools.cpp#l5069

Why not just stick clang in C90 mode when targeting C if the -fms-compatibility-version is 18?

We have similar code for the C++ mode in https://github.com/llvm-mirror/clang/blob/master/lib/Driver/Tools.cpp#l5069

David, thanks for looking into this!

MSVC18 doesn't support a set C standard; it adds some things from C99 as well. For example, it supports _Bool. So, I tried not to throw away baby along with bathwater and cause too much disruption.

OK -- will implement a driver fix that sets C90 for MSVC18 tomorrow and will update the patch.

Andrey

Why not just stick clang in C90 mode when targeting C if the -fms-compatibility-version is 18?

We have similar code for the C++ mode in https://github.com/llvm-mirror/clang/blob/master/lib/Driver/Tools.cpp#l5069

David, thanks for looking into this!

MSVC18 doesn't support a set C standard; it adds some things from C99 as well. For example, it supports _Bool. So, I tried not to throw away baby along with bathwater and cause too much disruption.

OK -- will implement a driver fix that sets C90 for MSVC18 tomorrow and will update the patch.

Andrey

_Bool is fine, our C90 support is a superset of C90: it includes things which a conforming implementation is permitted to provide.

However, the following test case is problematic:

void f() {
  for (int x = 0; x < 10; ++x) {}
  for (int x = 0; x < 10; ++x) {}
}

This is supported by MSVC 2013 but would be (correctly) rejected by a compiler in C90 mode...
I have a feeling that sticking us in C90 mode would break code...

OK, so is initial patch a good one? ;-)

Andrey

rnk edited edge metadata.Feb 17 2016, 11:03 AM

Thanks, but I don't think we should do this. There's already a lot of code out there (ffmpeg) that is already using clang its current form and it does stuff like this:

#if defined(_MSC_VER) && !defined(__clang__)
# define inline __inline
// More MSVC compatibility hacks unnecessary with clang here...
#endif

Any code using 'inline' as an identifier is going to have a hard time going forward. I think our current behavior is better for the majority of our users.

andreybokhanko abandoned this revision.Feb 18 2016, 2:35 AM

@rnk, fair enough. OK, please consider this review request dropped.

Andrey