This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Add an in-place version of toHex()
ClosedPublic

Authored by hans on Jan 10 2022, 10:50 AM.

Details

Summary

and use that to simplify MD5's hex string code which was previously using a string stream, as well as Clang's CGDebugInfo::computeChecksum().

NFC.

Diff Detail

Event Timeline

hans created this revision.Jan 10 2022, 10:50 AM
hans requested review of this revision.Jan 10 2022, 10:50 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 10 2022, 10:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hans updated this revision to Diff 398699.Jan 10 2022, 10:52 AM

Trying to upload the right diff this time.

hans updated this revision to Diff 398704.Jan 10 2022, 11:03 AM

(avoid toStringRef -> arrayRefFromStringRef by reshuffling the toHex overloads)

dexonsmith accepted this revision.Jan 10 2022, 12:07 PM

Mostly LGTM, just a couple of comments on the drive-by changes to hexdigit() (I might split that (and the new call in toHex()) into a separate commit, but if you keep them here please mention them in the commit message).

llvm/include/llvm/ADT/StringExtras.h
38–41

Should this be X < 16?

39

Can you use constexpr StringLiteral here?

This revision is now accepted and ready to land.Jan 10 2022, 12:07 PM
hans marked 2 inline comments as done.Jan 11 2022, 2:31 AM

Mostly LGTM, just a couple of comments on the drive-by changes to hexdigit() (I might split that (and the new call in toHex()) into a separate commit, but if you keep them here please mention them in the commit message).

Thanks! I'll commit the hexdigit() change separately.

llvm/include/llvm/ADT/StringExtras.h
38–41

D'oh, yes of course. Thanks for spotting that!

39

Yes that would work, but I think it's simpler to just use the built-in types here.
Actually, there's no need for this to be a pointer, it should just be an array. I'll fix that.

This revision was landed with ongoing or failed builds.Jan 11 2022, 2:32 AM
This revision was automatically updated to reflect the committed changes.
hans marked 2 inline comments as done.