Page MenuHomePhabricator

Add SourceManagerForFile helper which sets up SourceManager and dependencies for a single file with code snippet
ClosedPublic

Authored by ioeric on Apr 27 2018, 12:45 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.Apr 27 2018, 12:45 AM
ioeric edited reviewers, added: sammccall, klimek; removed: djasper.Apr 27 2018, 12:49 AM
ioeric updated this revision to Diff 144523.Apr 30 2018, 3:07 AM
[WIP][Tooling] Pull #include manipulation code from clangFormat into libToolingCore.
ioeric updated this revision to Diff 144524.Apr 30 2018, 3:08 AM
  • Revert last revision.
sammccall added inline comments.May 2 2018, 5:21 AM
include/clang/Tooling/Core/Environment.h
1 ↗(On Diff #144524)

I'm not sure about this abstraction - I can't tell what it represents, and what in future should/shouldn't be added to it.

  • The read API seems to be SourceManager& and FileID, and there doesn't seem to be much interaction. I think I'd usually find it easier to understand to have those two than an Environment.
  • the utility to create a self-contained source manager wrapping a single virtual file is useful, but seems like that's more specialized/narrower (struct SingleSourceManager?)
  • the fact that these virtual sourcemanagers are completely self-owning when passed as Environment seems maybe-convenient but also surprising and non-orthogonal (it's a maybe-owning-pointer by another name). Is this important somewhere?
ioeric updated this revision to Diff 145318.May 4 2018, 4:10 PM
  • Create SourceManagerForFile instead of Environment; put it in SourceManager.h which seems to be a better place.
ioeric retitled this revision from Pull helper class Environment from clangFormat into libToolingCore [NFC] to Add SourceManagerForFile helper which sets up SourceManager and dependencies for a single file with code snippet.May 4 2018, 4:12 PM
ioeric edited the summary of this revision. (Show Details)
ioeric added a comment.May 4 2018, 4:15 PM

Thanks for the comment and suggestion!

I've changed this to SourceManagerForFile and move it to SourceManager.h, which seems to be a more natural fit. SourceManagerForFile sounds like a sensible name to me, but I'm guess there might be a better name...

ioeric updated this revision to Diff 145386.May 5 2018, 2:27 PM
  • Replaced forward decl of DiagnosticsEngine with header include.
sammccall accepted this revision.May 7 2018, 1:42 AM
sammccall added inline comments.
include/clang/Basic/SourceManager.h
1819 ↗(On Diff #145386)

nit: single in-memory file

1824 ↗(On Diff #145386)

why is this not just a constructor? it looks like it can't fail?

1827 ↗(On Diff #145386)

this could also just be get() or operator*

lib/Format/TokenAnalyzer.h
71 ↗(On Diff #145386)

add a comment that this is only present if constructed from a string?

This revision is now accepted and ready to land.May 7 2018, 1:42 AM
ioeric updated this revision to Diff 146004.May 9 2018, 2:30 PM
ioeric marked 4 inline comments as done.
  • address review comments.
This revision was automatically updated to reflect the committed changes.