This is an archive of the discontinued LLVM Phabricator instance.

[clang] Use SourceLocation as key in hash maps, NFCI
ClosedPublic

Authored by miyuki on Nov 5 2019, 4:11 AM.

Details

Summary

The patch adjusts the existing llvm::DenseMap<unsigned, T> and
llvm::DenseSet<unsigned> objects that store source locations, so
that they use SourceLocation directly instead of unsigned.

This patch relies on the DenseMapInfo trait added in D89719.

It also replaces the construction of SourceLocation objects from
the constants -1 and -2 with calls to the trait's methods getEmptyKey
and getTombstoneKey where appropriate.

Diff Detail

Event Timeline

miyuki created this revision.Nov 5 2019, 4:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
miyuki edited reviewers, added: JDevlieghere; removed: jdoerfert.Nov 5 2019, 4:11 AM
miyuki updated this revision to Diff 227894.Nov 5 2019, 8:52 AM

Changed getRawEncoding -> getHashValue in Sema.h

What's the motivation behind this?

miyuki added a comment.Nov 5 2019, 9:18 AM

The motivation is to be able to make source locations' underlying type configurable. Richard Smith suggested that this might be feasible: http://lists.llvm.org/pipermail/cfe-dev/2019-October/063515.html
So, the first step is to get rid of getRawEncoding where possible. I think this would make the code cleaner anyway.

aprantl resigned from this revision.Dec 2 2019, 1:22 PM

@rsmith can you take a look at this to see if this is going into the right direction?

miyuki updated this revision to Diff 249443.Mar 10 2020, 10:32 AM

Rebased, updated several more hash map occurrences.

dexonsmith requested changes to this revision.Oct 16 2020, 12:48 PM

If you're still interested in pursuing this, I'm happy to review it.

A general comment is that there are a couple of functional changes in this patch, where hash values are changing and data structures are being changed, but they're buried in the noise of the refactoring that's going on at the same time. I suggest splitting this up at least into two, where most of the NFC changes are in a different commit.

clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.h
35 ↗(On Diff #249443)

Changing the data structure (DenseSet <= std::unordered_set) seems unrelated to the rest of the patch. If it's necessary / useful, then please do it separately. It'll be important to confirm that the users of MatchedTemplateLocations don't rely on the values having stable addresses.

clang/include/clang/Basic/SourceLocation.h
178 ↗(On Diff #249443)

I suggest reusing the hash function for unsigned.

This revision now requires changes to proceed.Oct 16 2020, 12:48 PM

Yes, I am still interested in working on this. For now, I've split out some changes that don't require any modifications in SourceLocation into https://reviews.llvm.org/D89705.

A general comment is that there are a couple of functional changes in this patch, where hash values are changing and data structures are being changed, but they're buried in the noise of the refactoring that's going on at the same time. I suggest splitting this up at least into two

Will do.

miyuki retitled this revision from [Basic] Make SourceLocation usable as key in hash maps, NFCI to [clang] Use SourceLocation as key in hash maps, NFCI.Oct 19 2020, 11:42 AM
miyuki edited the summary of this revision. (Show Details)
miyuki updated this revision to Diff 299126.Oct 19 2020, 12:19 PM
miyuki edited the summary of this revision. (Show Details)

Split out the refactoring part.

miyuki marked 2 inline comments as done.Oct 19 2020, 12:21 PM
This revision is now accepted and ready to land.Oct 19 2020, 2:24 PM
This revision was automatically updated to reflect the committed changes.