This is an archive of the discontinued LLVM Phabricator instance.

[MS Compat] Allow _Atomic(Type) and 'struct _Atomic' to coexist
ClosedPublic

Authored by majnemer on Jul 15 2015, 11:42 AM.

Details

Summary

MSVC 2013 ships, as part of its STL implementation, a class named
'_Atomic'. This is unfortunate because this keyword is in conflict with
the C11 keyword with the same name. Our solution was to disable this
keyword when targeting MSVC 2013 and reenable it for 2015.

However, this makes it impossible for clang's headers to make use of
_Atomic. This is problematic in the case of libc++ as it makes heavy
use of this keyword.

Let the keywordness of _Atomic float under certain circumstances:
the body of a class named _Atomic, or a class with a base specifier
named _Atomic, will not have the keyword variant of _Atomic for the
duration of the class body. This is sufficient to allow us to correctly
handle _Atomic in the STL while permitting us to use _Atomic as a
keyword everywhere else.

Diff Detail

Event Timeline

majnemer updated this revision to Diff 29807.Jul 15 2015, 11:42 AM
majnemer retitled this revision from to [MS Compat] Allow _Atomic(Type) and 'struct _Atomic' to coexist.
majnemer updated this object.
majnemer added reviewers: rsmith, rnk.
majnemer added a subscriber: cfe-commits.
rsmith added inline comments.Jul 15 2015, 11:55 AM
lib/Parse/ParseDeclCXX.cpp
1310–1325

This seems a lot more general than you need: can you just make the set function revert to identifier and the destructor revert to tok::kw__Atomic?

1311

Maybe use a constructor with a bool Enabled rather than a setter?

1327–1339

I think this can be simplified to:

if (getLangOpts().MSVCCompat && Tok.is(tok::kw__Atomic) && TagType == DeclSpec::TST_struct)
1334

Please add a comment explaining why you're doing this. We conventionally use a comment containing "HACK" for workarounds for standard library issues. (Though we're missing that in the comment above, in the call to TryKeywordIdentFallback.)

1892–1897

You don't appear to have any testcases for this.

test/SemaCXX/MicrosoftCompatibility.cpp
12–13

Please add a testcase that uses _Atomic as a keyword after it's defined as a type. Please also add a testcase showing the uses of the _Atomic type that we need to be compatible with (as a base class, maybe?).

rnk edited edge metadata.Jul 15 2015, 11:56 AM

This seems like a better solution. I assume you manually tested with MSVC's <atomic>.

test/SemaCXX/MicrosoftCompatibility.cpp
13

Did you want to add a test for the base specifier case?

majnemer added inline comments.Jul 15 2015, 2:46 PM
lib/Parse/ParseDeclCXX.cpp
1310–1325

If I unconditionally revert to kw__Atomic, I will not correctly handle cases where ParseClassSpecifier is called recursively. If this happens, the _Atomic identifier will have getTokenID() already set to tok::identifier and we should let the caller's RAII object revert it back to kw__Atomic instead.

1334

Sure, done.

1892–1897

Fixed.

test/SemaCXX/MicrosoftCompatibility.cpp
12–13

Done.

13

Done.

majnemer updated this revision to Diff 29836.Jul 15 2015, 2:46 PM
majnemer edited edge metadata.
  • Address review comments
rsmith added inline comments.Jul 15 2015, 3:59 PM
lib/Parse/ParseDeclCXX.cpp
1310–1325

Oh, I see what you're doing; this also restores the change to the IdentifierInfo's kind when parsing the base specifier. I'm not really happy with that approach. Saving and restoring the kind of the _Atomic identifier while parsing struct _Atomic seems OK; doing it when parsing every single struct does not seem targeted enough.

Instead, I suggest the following:

  • Only change the type of _Atomic within the definition of struct _Atomic
  • When parsing a base specifier and you see _Atomic, change just that one token to an identifier
  • When parsing a declaration and you see _Atomic, check whether (a) you're in a typedef, and (b) you've not got a type yet, and (c) the token after the next token is a semicolon, and if so, convert that _Atomic token to an identifier
1892–1894

This can be simplified in the same way as the previous check.

1895

This could be null. But you can just grab the identifier info from Tok (both here and above).

majnemer added inline comments.Jul 15 2015, 4:12 PM
lib/Parse/ParseDeclCXX.cpp
1310–1325

I can implement your suggestion but I don't think it will work without some tweaks. What I was accomplishing with my aggressive handling of _Atomic was getting typedef _Atomic<_Ty, sizeof (_Ty)> _My_base; handled "correctly".

The next token after _Atomic in that typedef is not a semicolon, it is a tok::less.

If we are going with this implementation technique, I don't think I even need Ident__Atomic anymore.

1892–1894

Sure.

1895

Sure.

majnemer updated this revision to Diff 29855.Jul 15 2015, 5:05 PM
  • Address review comments
  • Address Richard's latest comments.
rsmith accepted this revision.Jul 22 2015, 4:26 PM
rsmith edited edge metadata.

LGTM

lib/Parse/ParseDeclCXX.cpp
1898

GetLookAheadToken(1) is more commonly written as NextToken().

This revision is now accepted and ready to land.Jul 22 2015, 4:26 PM
This revision was automatically updated to reflect the committed changes.