This is an archive of the discontinued LLVM Phabricator instance.

[Support] Reduce Dependence on Host.h
ClosedPublic

Authored by lenary on Nov 11 2022, 6:39 AM.

Details

Summary

The intention behind this commit is to reduce the use of Host.h/Host.cpp
in Support, to where it is only necessary.

In this case, the endian-detection and support functionality needed by
these implementations can be provided by Support/SwapByteOrder.h in a
cleaner manner.

Diff Detail

Event Timeline

lenary created this revision.Nov 11 2022, 6:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 6:39 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
lenary requested review of this revision.Nov 11 2022, 6:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2022, 6:39 AM
dexonsmith added inline comments.Nov 11 2022, 7:54 AM
llvm/lib/Support/SHA1.cpp
191–194

Can we use if constexpr here? Or is that not supported by all compilers yet?

193

I think usual LLVM style only uses braces when there are multiple lines on at least one side.

tmatheson accepted this revision.Nov 11 2022, 7:58 AM
This revision is now accepted and ready to land.Nov 11 2022, 7:58 AM
lenary added inline comments.Nov 11 2022, 8:20 AM
llvm/lib/Support/SHA1.cpp
191–194

I forgot this was something we could do, but LLVM is C++17, so it should be allowed, I believe.

193

Ack

MaskRay added a subscriber: MaskRay.EditedNov 11 2022, 11:20 AM

llvm/Support/Endian.h read<T, little, aligned>(...) is likely better API for many code pieces in this file. Or use read32le if the alignment optimization doesn't really matter.

llvm/Support/Endian.h read<T, little, aligned>(...) is likely better API for many code pieces in this file. Or use read32le if the alignment optimization doesn't really matter.

I'll look into this as a follow up. I was really only doing it to make moving Host.h out of llvm/Support easier.

MaskRay accepted this revision.Nov 11 2022, 4:33 PM

LGTM.

lenary updated this revision to Diff 475859.Nov 16 2022, 10:08 AM
lenary marked 2 inline comments as done.Nov 16 2022, 10:08 AM
MaskRay accepted this revision.Nov 16 2022, 3:39 PM

Hi - I have noticed that there are tabes indenting the code I stead os spaces. Is this clang-format doing it?

Francesco

llvm/lib/Support/SHA256.cpp
246–252

Tabs?

253

This is an improvement that is unrelated to making libSupport less dependent on Host.cpp. I'd suggest to either mention the extra change in the commit message, or factor it out as a separate commit (up to you).

lenary marked an inline comment as done.Nov 17 2022, 1:49 AM
lenary added inline comments.
llvm/lib/Support/SHA256.cpp
246–252

No, I've indented with spaces. The chevrons are in the margin, and are Phabricator's way of showing that for most of these lines, only the indentation changed.

253

I'll mention it in the message when I commit this.

fpetrogalli accepted this revision.Nov 17 2022, 2:34 AM

LGTM. Thank you.

llvm/lib/Support/SHA256.cpp
246–252

Thank you for explaining.

253

SGTM.

This revision was landed with ongoing or failed builds.Nov 21 2022, 10:29 AM
This revision was automatically updated to reflect the committed changes.
lenary marked an inline comment as done.