[PATCH] D154285: [clang] Deprecate CGBuilderTy::CreateElementBitCast

Sat Jul 1 13:06:57 PDT 2023

jrtc27 added inline comments.

================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3896
     llvm::Type *OrigBaseElemTy = Addr.getElementType();
-    Addr = Builder.CreateElementBitCast(Addr, Int8Ty);

----------------
JOE1994 wrote:
> barannikov88 wrote:
> > jrtc27 wrote:
> > > This one isn't a direct substitution, although appears to be refactoring the code to not need withElementType in the first place? Probably best to separate that change out from the very mechanical substitution.
> > It will be difficult to find all such places later.
> @jrtc27 Thank you for taking the time to review this revision.
> 
> This isn't a direct substitution.
> In this revision overall, I did refactoring to get rid of unnecessary calls to `withElementType()` when possible.
> 
> Since these are relatively small & local refactorings, I'd prefer to keep them in this revision.
> But if you're strongly against including such refactorings in this revision, I can separate them out to a new revision.
It's easier to review when the non-trivial changes (direct substitution) are kept separate from the trivial ones. That way it's also extremely clear what's intended to be different vs what was accidentally made different. You *at least* need to fix the commit message to say that you're not just replacing the in-tree uses, you're also refactoring some uses to not need it (currently your commit message is in direct contradiction with the diff), but the fact that the commit then does two things that can be reasonably separated out suggests that they should be. And I would much rather see that happen, especially from the perspective of maintaining a significant downstream fork.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285