This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)
ClosedPublic

Authored by nikic on Jun 1 2021, 1:45 PM.

Details

Summary

This is a followup to D103422. The DenseMapInfo implementations for ArrayRef and StringRef are moved into the ArrayRef.h and StringRef.h headers, which means that these two headers no longer need to be included by DenseMapInfo.h.

This required adding quite a few additional includes, as many files were relying on various things pulled in by ArrayRef.h.

Diff Detail

Event Timeline

nikic created this revision.Jun 1 2021, 1:45 PM
nikic requested review of this revision.Jun 1 2021, 1:45 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 1 2021, 1:45 PM
lattner accepted this revision.Jun 1 2021, 2:54 PM

This required adding quite a few additional includes, as many files were relying on various things pulled in by ArrayRef.h.

This is a _good_ thing btw!

Thank you for driving this!

llvm/include/llvm/ADT/ArrayRef.h
595–600

I'm pretty sure this method can just be "return LHS == RHS;" The tombstone/empty comparisons should work without special cases.

llvm/lib/CodeGen/MBFIWrapper.cpp
14

This clang format warning is right, plz move under the other #include

llvm/lib/Support/SmallPtrSet.cpp
15

These are also right, plz fix

This revision is now accepted and ready to land.Jun 1 2021, 2:54 PM
craig.topper added inline comments.
llvm/include/llvm/ADT/ArrayRef.h
595–600

Doesn't operator== on ArrayRef compare the elements of the arrays? So wouldn't that dereference the invalid pointers used by tombstone/empty?

lattner added inline comments.Jun 1 2021, 4:04 PM
llvm/include/llvm/ADT/ArrayRef.h
595–600

Yes, implemented in terms of std::equal. However, both of these cases have zero elements, so the "pointer" is never dereferenced.

nikic added inline comments.Jun 2 2021, 12:20 AM
llvm/include/llvm/ADT/ArrayRef.h
595–600

As the length is zero, wouldn't the empty key, the tombstone key and an empty ArrayRef all be considered equal, as the data pointer is never compared?

RKSimon accepted this revision.Jun 2 2021, 2:10 AM

Thanks for dealing with this @nikic

For this refactor, keeping the implementation as close to the existing one makes more sense; also personally I'd prefer the missing include fixups to be committed first as NFCs but its up to you.

lattner added inline comments.Jun 2 2021, 10:58 AM
llvm/include/llvm/ADT/ArrayRef.h
595–600

Yes, you're right, this won't work. Good catch, thanks :-)

This revision was landed with ongoing or failed builds.Jun 3 2021, 9:34 AM
This revision was automatically updated to reflect the committed changes.