[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:       Richard Smith <richard () metafoo ! co ! uk>
Date:       2015-07-15 18:55:53
Message-ID: 0dabbfbf580aef8030aed7bab347e541 () localhost ! localdomain
[Download RAW message or body]

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




_______________________________________________
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]