Page MenuHomePhabricator

Make UniqueCStringMap work with non-default-constructible types and other improvements/cleanups

Authored by labath on Jun 13 2019, 7:38 AM.



The motivation for this was me wanting to make the validity of dwarf
DIERefs explicit (via llvm::Optional<DIERef>). This meant that the class
would no longer have a default constructor. As the DIERef was being
stored in a UniqueCStringMap, this meant that this container (like all
standard containers) needed to work with non-default-constructible types

This part is achieved by removing die default constructors for the map
entry types, and providing appropriate comparison overloads so that we
can search for map entries without constructing a dummy entry. While
doing that, I took the opportunity to modernize the code, and add some
tests. Functions that were completely unused are deleted.

This required also some changes in the Symtab code, as it was default
constructing map entries, which was not impossible even though its
value type was default-constructible. Technically, these changes could
be avoided with some SFINAE on the entry type, but I felt that the code
is cleaner this way anyway.

Event Timeline

labath created this revision.Jun 13 2019, 7:38 AM
clayborg added inline comments.

What benefits does llvm::lower_bound offer here? With std::lower_bound and std::upper_bound you can write a comparison static functions that take a "const T &" on one side and a ConstString on the other. Lower bound will use one flavor ("static bool operator <(const T&, ConstString);") and upper_bound will use the other ("static bool operator <(ConstString, const T&);"). No need to specify a Compare() object. So the code would be:

T Find(ConstString unique_cstr, T fail_value) const {
  const_iterator end = m_map.end();
  const_iterator pos = std::lower_bound(m_map.begin(), end, unique_cstr);
  if (pos != end && pos->cstring == unique_cstr)
    return pos->value;
  return fail_value;
labath marked an inline comment as done.Jun 13 2019, 8:26 AM
labath added inline comments.

The only benefit of using llvm lower/upper bound is that operates on ranges -- you don't need to pass the begin/end iterator pairs, you can just pass the container. Under the hood, it just delegates to the std version.

The issue of using the compare object is completely orthogonal to that. I could define appropriate operator<s on the Entry class directly, and thereby avoid the need for comparison objects. The reason I did not do that is because the Entry class is public (and other parts of the code use and manipulate it), and having a (ConstString, Entry) operator< on the Entry class would mean exposing it to the outside world.. It's not a strong reason, it seemed to me like this is the kind of thing that users outside of this class should not be able to do implicitly/accidentally, so I tried to hide it a bit.

clayborg accepted this revision.Jun 13 2019, 8:48 AM
This revision is now accepted and ready to land.Jun 13 2019, 8:48 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 11:30 PM

It looks like this broke the Windows bot. I am no longer receiving emails when the bot fails (as bot owner I used to always get the failure emails), so I am assuming for some reason notifications aren't working correctly.

Can you have a look at the failure?

Thanks for the heads up. This should be fixed with r363653.