[PATCH] D11233: [MS Compat] Allow _Atomic(Type) and 'struct _Atomic' to coexist

[Richard Smith]

rsmith added inline comments.

================
Comment at: lib/Parse/ParseDeclCXX.cpp:1310-1325
@@ -1309,1 +1309,18 @@
 
+  struct PreserveIdentifierInfoRAII {
+    void set(IdentifierInfo *ToPreserve) {
+      II = ToPreserve;
+      TK = II->getTokenID();
+    }
+    ~PreserveIdentifierInfoRAII() {
+      if (II) {
+        if (TK == tok::identifier && II->getTokenID() != tok::identifier)
+          II->RevertTokenIDToIdentifier();
+        else if (TK != tok::identifier && II->getTokenID() == tok::identifier)
+          II->RevertIdentifierToTokenID(TK);
+      }
+    }
+    IdentifierInfo *II = nullptr;
+    tok::TokenKind TK;
+  };
+
----------------
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`?

================
Comment at: lib/Parse/ParseDeclCXX.cpp:1311
@@ +1310,3 @@
+  struct PreserveIdentifierInfoRAII {
+    void set(IdentifierInfo *ToPreserve) {
+      II = ToPreserve;
----------------
Maybe use a constructor with a `bool Enabled` rather than a setter?

================
Comment at: lib/Parse/ParseDeclCXX.cpp:1327-1339
@@ +1326,15 @@
+
+  PreserveIdentifierInfoRAII AtomicTokenGuard;
+  if (getLangOpts().MSVCCompat) {
+    if (!Ident__Atomic)
+      Ident__Atomic = &PP.getIdentifierTable().get("_Atomic");
+    AtomicTokenGuard.set(Ident__Atomic);
+  }
+
+  if (getLangOpts().MSVCCompat && TagType == DeclSpec::TST_struct &&
+      Tok.isNot(tok::identifier) && !Tok.isAnnotation() &&
+      Tok.getIdentifierInfo() && Tok.is(tok::kw__Atomic)) {
+    Ident__Atomic->RevertTokenIDToIdentifier();
+    Tok.setKind(tok::identifier);
+  }
+
----------------
I think this can be simplified to:

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

================
Comment at: lib/Parse/ParseDeclCXX.cpp:1334
@@ +1333,3 @@
+
+  if (getLangOpts().MSVCCompat && TagType == DeclSpec::TST_struct &&
+      Tok.isNot(tok::identifier) && !Tok.isAnnotation() &&
----------------
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`.)

================
Comment at: lib/Parse/ParseDeclCXX.cpp:1892-1897
@@ -1860,2 +1891,8 @@
   // Parse the class-name.
+  if (getLangOpts().MSVCCompat && Tok.isNot(tok::identifier) &&
+      !Tok.isAnnotation() && Tok.getIdentifierInfo() &&
+      Tok.is(tok::kw__Atomic)) {
+    Ident__Atomic->RevertTokenIDToIdentifier();
+    Tok.setKind(tok::identifier);
+  }
   SourceLocation EndLocation;
----------------
You don't appear to have any testcases for this.

================
Comment at: test/SemaCXX/MicrosoftCompatibility.cpp:12-13
@@ -11,7 +11,4 @@
 
-#if _MSC_VER >= 1900
 _Atomic(int) z;
-#else
 struct _Atomic {};
 
----------------
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?).


http://reviews.llvm.org/D11233