Page MenuHomePhabricator

[clang][Tooling] Get rid of a hack in SymbolOccurrences, NFCI
ClosedPublic

Authored by miyuki on Jan 13 2021, 7:03 AM.

Details

Summary

The class SymbolOccurrences can store either a single SourceRange
in-place or multiple SourceRanges on the heap. In the latter case
the number of source ranges is stored in the internal representation
of the beginning SourceLocation of the in-place SourceRange
object.

This change gets rid of such hack by placing SourceRange in a union
which holds either a valid SourceRange or an unsigned int (a number
of ranges).

The change also adds static_asserts that check that SourceRange and
SourceLocation are trivially destructible (this is required for the
current patch and for D94237 which has already been committed).

Diff Detail

Event Timeline

miyuki requested review of this revision.Jan 13 2021, 7:03 AM
miyuki created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2021, 7:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MarkMurrayARM accepted this revision.Jan 19 2021, 3:14 AM
This revision is now accepted and ready to land.Jan 19 2021, 3:14 AM

I will wait until the end of this week to see if anyone has objections.

simon_tatham added inline comments.
clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
81

This surely relies on SourceRange having no destructor (or rather, a trivial one). If that ever changes, then destruction of this class will risk either a spurious call to the destructor or a missing one.

Is there any way to somehow arrange that a build failure will occur if the definition of SourceRange changes in that way?

simon_tatham added inline comments.Jan 19 2021, 5:16 AM
clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
81

... aha, found one. Such as a static assertion that std::is_trivially_destructible<clang::SourceLocation>::value is 1.

miyuki updated this revision to Diff 317627.Jan 19 2021, 10:52 AM
miyuki edited the summary of this revision. (Show Details)

Added static_asserts that check that SourceRange and SourceLocation are trivially destructible.

miyuki marked 2 inline comments as done.Jan 19 2021, 10:53 AM
simon_tatham accepted this revision.Jan 20 2021, 1:24 AM

Thanks. LGTM now.