[PATCH] D138295: [clang][TargetInfo] Use LangAS for getPointer{Width,Align}()

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1917
+    EffectiveFieldSize = FieldSize = TI.Width;
+    FieldAlign = TI.Align;
   } else {
----------------
rjmccall wrote:
> This is a nice cleanup, but I actually can't figure out why we can't just fall into the code below.
It looks like this dates back to `Add a new ASTRecordLayoutBuilder class. Not used yet.` from 2009. Maybe `ReferenceType`s were not handled correctly by `Context.getTypeInfoInChars(RT);` back then?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1917
+    EffectiveFieldSize = FieldSize = TI.Width;
+    FieldAlign = TI.Align;
   } else {
----------------
arichardson wrote:
> rjmccall wrote:
> > This is a nice cleanup, but I actually can't figure out why we can't just fall into the code below.
> It looks like this dates back to `Add a new ASTRecordLayoutBuilder class. Not used yet.` from 2009. Maybe `ReferenceType`s were not handled correctly by `Context.getTypeInfoInChars(RT);` back then?
I've removed the special case now and it looks like all tests are passing, so I believe this is indeed no longer needed.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1706
+    assert(ThisPtrTy->getPointeeType().getAddressSpace() == LangAS::Default &&
+           "this pointer not in default address space?");
     auto Align = getTypeAlignIfRequired(ThisPtrTy, CGM.getContext());
----------------
rjmccall wrote:
> I'm pretty sure we have language modes that support methods in different address spaces, and your code doesn't seem to require this assertion.
I added this assertion to see if we have any tests for this case and forgot to remove it. I think it should hopefully be correct, so I've removed the assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138295