Page MenuHomePhabricator

Make ASTFileSignature an array of 20 uint8_t instead of 5 uint32_t
ClosedPublic

Authored by dang on Jun 7 2020, 12:06 PM.

Diff Detail

Event Timeline

dang created this revision.Jun 7 2020, 12:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2020, 12:06 PM
dang marked an inline comment as done.Jun 7 2020, 12:11 PM
dang added inline comments.
clang/include/clang/Basic/Module.h
90

I couldn't find anywhere in the code base that checks the bit-pattern so I left it as is. If anyone knows if someone could point me to the code that checks this (if any code does check it) that would be nice.

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

clang/include/clang/Basic/Module.h
29

Can you move the code that depends on algorithm into a .cpp file?

90

The PCM hash is reused in CGDebugInfo as the DW_AT_dwo_id hash for the -gmodules debug info. This value is checked by dsymutil and LLDB.

dexonsmith accepted this revision.Jun 8 2020, 10:22 AM

LGTM once @aprantl is happy.

This revision is now accepted and ready to land.Jun 8 2020, 10:22 AM
dang updated this revision to Diff 269468.Jun 9 2020, 3:09 AM
dang marked an inline comment as done.

Address @aprantl's comments

dang updated this revision to Diff 269473.Jun 9 2020, 3:16 AM
dang marked 3 inline comments as done.

Fix bug

clang/include/clang/Basic/Module.h
29

That code is the call to std::copy in the template function so it can't really be moved out... But I removed the include because STLExtras.h already includes it, so I think it is ok to remove the include.

90

DW_AT_dwo_id is 64 bit so using 0xFF everywhere is fine.

dang updated this revision to Diff 269845.Jun 10 2020, 7:41 AM

Missed a case where an ASTFileSignature was created using an explicit std::copy

aprantl accepted this revision.Jun 10 2020, 12:48 PM
This revision was automatically updated to reflect the committed changes.

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).

clang/include/clang/Basic/Module.h
57

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.

61

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)