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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
