[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D11233: [MS Compat] Allow _Atomic(Type) and 'struct _Atomic' to coexist
From:       David Majnemer <david.majnemer () gmail ! com>
Date:       2015-07-15 21:46:10
Message-ID: e1e48bccf28d7e73bc5fdecc5bfaf49e () localhost ! localdomain
[Download RAW message or body]

majnemer 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;
+  };
+
----------------
rsmith wrote:
> 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`?
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.

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

================
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;
----------------
rsmith wrote:
> You don't appear to have any testcases for this.
Fixed.

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

================
Comment at: test/SemaCXX/MicrosoftCompatibility.cpp:13
@@ -13,5 +12,3 @@
 _Atomic(int) z;
-#else
 struct _Atomic {};
 
----------------
rnk wrote:
> Did you want to add a test for the base specifier case?
Done.


http://reviews.llvm.org/D11233




_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]