[PATCH] D127165: [DirectX][MC] Add MC support for DXContainer
Chris Bieneman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 7 09:14:26 PDT 2022
beanz marked 2 inline comments as done.
beanz added inline comments.
================
Comment at: llvm/lib/MC/MCDXContainerWriter.cpp:75
+ // Write the header
+ W.write<char>({'D', 'X', 'B', 'C'});
+ // Hash 16-bytes of 0's
----------------
kuhar wrote:
> beanz wrote:
> > kuhar wrote:
> > > nit: This is probably pedantic, but I think technically characters don't have to use ASCII on all platforms and it would be safer to use exact hex values.
> > So... I will concede that technically someone could attempt to compile LLVM for a non-ASCII platform, and this _could_ be a problem. If someone were to use this on a non-ASCII platform this would be the least of our problems because we do a lot of string-compares on binary data, and we generally encode section names and other "strings" as ASCII.
> >
> > How about a happy middle ground, I can make these explicit UTF-8 constants. I prefer the readability of characters rather than needing to keep an ASCII chart handy.
> Ack. I think both of your options are practical. Thanks for explaining.
Gah! UTF-8 character literals are C++17 :(
Since we do have other places (like the ELF ZILB constant) where we assume character literals are ASCII, and I'm completely unaware of any non-ASCII platform that supports enough memory to build or run LLVM, I'm going to prefer to keep this as characters.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127165/new/
https://reviews.llvm.org/D127165
More information about the llvm-commits
mailing list