[PATCH] D110670: [Sema] Allow comparisons between different ms ptr size address space types.

[Amy Huang via Phabricator via cfe-commits]

akhuang added inline comments.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:6682
+              isPtrSizeAddressSpace(Q2.getAddressSpace()))
+            MaybeQ1 = true;
+          else
----------------
aaron.ballman wrote:
> akhuang wrote:
> > aaron.ballman wrote:
> > > I'm pretty sure this is correct based on my inspection of what code MSVC is generating. But it would be helpful to have some codegen tests in Clang for this functionality as well.
> > ha, I apparently didn't check that the behavior actually matches.. apparently in MSVC a ptr32 isn't equivalent to a ptr64
> Oh! I had tested this:
> ```
> int test1(int * __ptr32 __uptr p32u, int * __ptr32 __sptr p32s,
>                          int * __ptr64 p64) {
>   return (p32u == p64);
> }
> 
> int test2(int * __ptr32 __uptr p32u, int * __ptr32 __sptr p32s,
>                          int * __ptr64 p64) {
>   return (p64 == p32u);
> }
> ```
> to see whether the order of the operands mattered as to which conversion "won" and I thought I saw that your patch generates the same code that MSVC does. However, I could have messed my testing up somehow, so double-checking is a good idea.
Whoops, you're right, I was not specifying __sptr/__uptr and I guess clang and msvc have different defaults for sign/zero extension. 

Will add tests. Also, thanks for reviewing! 


================
Comment at: clang/test/Sema/MicrosoftExtensions.cpp:9-10
+  return (p32u == p32s) +
+         (p32u == p64) +
+         (p32s == p64);
+}
----------------
aaron.ballman wrote:
> (Side question, not directly about this patch but sorta related.) Should there be a diagnostic about conversion potentially causing a possible loss of data (on the 64-bit target)?
Hmm, it seems reasonable, but I also don't know how motivated I am to add a diagnostic -- 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110670