Page MenuHomePhabricator

[clang][Basic] Integrate SourceLocation with FoldingSet, NFCI
ClosedPublic

Authored by miyuki on Nov 5 2019, 7:05 AM.

Details

Summary

This patch removes the necessity to access the SourceLocation internal
representation in several places that use FoldingSet objects.

Diff Detail

Unit TestsFailed

TimeTest
400 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

miyuki created this revision.Nov 5 2019, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2019, 7:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
miyuki updated this revision to Diff 249444.Mar 10 2020, 10:34 AM
miyuki retitled this revision from [Basic] Integrate SourceLocation with FoldingSet, NFCI to [Basic] Integrate SourceLocation and SourceRange with FoldingSet, NFCI.

Rebased, added similar integration for SourceRange

dexonsmith requested changes to this revision.Oct 19 2020, 2:30 PM
dexonsmith added a reviewer: dexonsmith.
dexonsmith added a subscriber: dexonsmith.
dexonsmith added inline comments.
clang/include/clang/Basic/SourceLocation.h
183–184

Seems a bit strange to me to add SourceLocation::Profile. Is there another hook available for FoldingSet that works with free functions, or template specializations? You could make that hook a friend in order to give it access to the internals. Or can the caller just use getHashValue()?

243–245

I'm not sure you need this change since you don't need to give access to internals here.

miyuki updated this revision to Diff 300044.Oct 22 2020, 11:01 AM
miyuki retitled this revision from [Basic] Integrate SourceLocation and SourceRange with FoldingSet, NFCI to [clang][Basic] Integrate SourceLocation with FoldingSet, NFCI.
miyuki edited the summary of this revision. (Show Details)

Addressed review comments

miyuki marked 2 inline comments as done.Oct 22 2020, 11:02 AM
miyuki updated this revision to Diff 300049.EditedOct 22 2020, 11:10 AM

Reverted the accidentally removed chunk.

miyuki updated this revision to Diff 300214.Oct 23 2020, 3:44 AM

Fixed linter errors.

dexonsmith added inline comments.Oct 23 2020, 10:21 AM
clang/lib/Analysis/PathDiagnostic.cpp
1088

I'm surprised you need this static_cast, can you explain why it was necessary?

martong removed a subscriber: martong.Oct 26 2020, 2:59 AM
miyuki added inline comments.Oct 26 2020, 6:57 AM
clang/lib/Analysis/PathDiagnostic.cpp
1088

Loc is an object of type FullSourceLoc, a class derived from SourceLocation. When the FoldingSetNodeID::Add template method is called with Loc it tries to instantiate FoldingSetTrait<FullSourceLoc> and fails (because there is no explicit specialization for FullSourceLoc and the default one relies on the method FullSourceLoc::Profile which does not exist). FullSourceLoc does not contain any additional information about the location itself (it just holds a pointer to the associated SourceManager), so there is no need to specialize FoldingSetTrait for it, and casting to SourceLocation will make FoldingSetNodeID::Add use the correct FoldingSetTrait specialization.

dexonsmith accepted this revision.Oct 26 2020, 8:37 AM

LGTM.

clang/lib/Analysis/PathDiagnostic.cpp
1088

Right, that makes sense. Thanks for explaining!

This revision is now accepted and ready to land.Oct 26 2020, 8:37 AM
This revision was landed with ongoing or failed builds.Oct 27 2020, 3:44 AM
This revision was automatically updated to reflect the committed changes.