[PATCH] D71039: Add support for the MS qualifiers __ptr32, __ptr64, __sptr, __uptr.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Sorry for the delay, overall this seems like the right approach.



================
Comment at: clang/include/clang/AST/Type.h:477-479
+           ((isPtrSizeAddressSpace(A) && B == LangAS::Default) ||
+            (isPtrSizeAddressSpace(B) && A == LangAS::Default) ||
+            (isPtrSizeAddressSpace(A) && isPtrSizeAddressSpace(B)));
----------------
Can this be simplified to:
  ((isPtrSizeAddressSpace(A) || A == LangAS::Default) &&
   (isPtrSizeAddressSpace(B) || B == LangAS::Default))
Mainly I wanted to avoid recomputing isPtrSizeAddressSpace for A and B.

I think it's only not equivalent when A and B are both default, but we already return true in that case.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1874-1881
+    case LangAS::ptr32_sptr:
+      Extra.mangleSourceName("_ASPtr32_sptr");
+      break;
+    case LangAS::ptr32_uptr:
+      Extra.mangleSourceName("_ASPtr32_uptr");
+      break;
+    case LangAS::ptr64:
----------------
This code should be unreachable because you check for these address spaces at the call site. I think you can do something like this:
  case LangAS::...:
  case LangAS::...:
  case LangAS::...:
    llvm_unreachable("don't mangle ptr address spaces with _AS");


================
Comment at: clang/lib/AST/TypePrinter.cpp:1824
+      case LangAS::ptr32_sptr:
+        OS << "__ptr32_sptr";
+        break;
----------------
Think we should say `"__sptr __ptr32"`? This code doesn't guarantee that it can be parsed back as valid source, but it's closer.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:3156
+
+static bool HasSameFunctionTypeIgnoringPointerSizes(ASTContext &Ctx,
+                                                    QualType Old,
----------------
I wonder if the simplest way to express this would be to follow the pattern of getFunctionTypeWithExceptionSpec and hasSameFunctionTypeIgnoringExceptionSpec, i.e. make a function that strips pointer sized address spaces off of pointer typed arguments, returns it, and then compare them. ASTContext would be a natural place for that kind of type adjustment.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:2890
 
+static QualType RemovePtrSizeAddrSpace(ASTContext &Ctx, QualType T) {
+  if (const PointerType *Ptr = T->getAs<PointerType>()) {
----------------
I think it would be fair to raise this method up to ASTContext, next to getAddrSpaceQualType, removeAddrSpaceQualType, etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71039/new/

https://reviews.llvm.org/D71039