[PATCH] D81347: Make ASTFileSignature an array of 20 uint8_t instead of 5 uint32_t

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D81347#2080217 <https://reviews.llvm.org/D81347#2080217>, @aprantl wrote:

> I was going to ask why make this change, but looking at the patch, it's pretty obvious :-)

Might be worth writing it down for everyone else - isn't exactly obvious to me (though not a part of the codebase I'm particularly familiar with).



================
Comment at: clang/include/clang/Basic/Module.h:57
+struct ASTFileSignature : std::array<uint8_t, 20> {
+  using BaseT = std::array<uint8_t, 20>;
+
----------------
Looks like "Base" is the more common name for this purpose (rather than "BaseT") across the LLVM project. BaseT makes me think (when I'm reading the rest of the code) that's it's a template parameter.


================
Comment at: clang/include/clang/Basic/Module.h:61
+
+  ASTFileSignature(BaseT S = {{0}}) : BaseT(std::move(S)) {}
+
----------------
Bit of a big structure to pass by value - should it be by const ref instead? (existing problem with the old code anyway, so no big deal either way)

(also the std::move is a no-op here because std::array<uint32_t> has a move that's the same as copy, though I don't mind it out of habit/consistency)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81347