Page MenuHomePhabricator

[clang-tidy] Reuse FileID in getLocation
ClosedPublic

Authored by chh on Mar 27 2017, 2:43 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

chh created this revision.Mar 27 2017, 2:43 PM
xazax.hun added inline comments.
clang-tidy/ClangTidy.cpp
246 ↗(On Diff #93179)

I do not know how important the efficiency here. But you do not need to store strings (and allocate buffers). You could get the FileEntry from the FileID and query the name (which will give you a StringRef).

After that modification, StringRef is relatively small, so maybe the map is a good candidate to be a DenseMap.

chh updated this revision to Diff 93284.Mar 28 2017, 1:00 PM

Use DenseMap<StringRef, FileID>.

chh marked an inline comment as done.Mar 28 2017, 1:02 PM
chh added inline comments.
clang-tidy/ClangTidy.cpp
246 ↗(On Diff #93179)

Now replaced with DenseMap<StringRef, FileID>.

alexfh requested changes to this revision.Mar 30 2017, 9:19 AM

Thank you for finding out the root cause here!

clang-tidy/ClangTidy.cpp
241 ↗(On Diff #93284)

Repeating lookup three times is not necessary:

FileID &ID = Path2FileID[FilePath];
if (!ID.isValid()) {
  const FileEntry *File = SourceMgr.getFileManager().getFile(FilePath);
  ID = SourceMgr.createFileID(File, SourceLocation(), SrcMgr::C_User);
}

But even this seems to be unnecessary, since SourceManager provides a better API that we should have used here:

FileID ID = SourceMgr.getOrCreateFileID(File, SrcMgr::C_User);
256 ↗(On Diff #93284)

llvm::StringMap<FileID> would be a better container, if we needed it (see the comment above).

This revision now requires changes to proceed.Mar 30 2017, 9:19 AM
chh updated this revision to Diff 93499.Mar 30 2017, 9:55 AM
chh edited edge metadata.
chh marked an inline comment as done.

Use getOrCreateFileID.

chh marked an inline comment as done.Mar 30 2017, 9:56 AM
alexfh accepted this revision.Mar 30 2017, 2:55 PM

LG
Do you have commit rights?

This revision is now accepted and ready to land.Mar 30 2017, 2:55 PM
This revision was automatically updated to reflect the committed changes.
chh reopened this revision.Mar 31 2017, 2:53 PM

This was reverted due to one failed test case clang-tidy/llvm-include-order.cpp on Windows:

Assertion failed: EndColNo <= map.getSourceLine().size() && "Invalid range!", file C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\lib\Frontend\TextDiagnostic.cpp, line 999

This revision is now accepted and ready to land.Mar 31 2017, 2:53 PM
This revision was automatically updated to reflect the committed changes.