[PATCH] D69854: [clang-format] [RELAND] Remove the dependency on frontend

Wed Nov 6 05:48:55 PST 2019

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

Thanks! One comment (the stale pointer one) and a more general suggestion (potentially for a future change) below, else I think this is good to go.

================
Comment at: clang/tools/clang-format/ClangFormat.cpp:310
   SourceManager Sources(*Diags, Files);
   FileID FileID = createInMemoryFile(AssumedFileName, Code.get(), Sources,
                                      Files, InMemoryFileSystem.get());
----------------
Maybe not for this change, but do you need all this machinery up here? SMDiagnostics takes a pointer and a file name, so yu can just pass Code->getBufferStart() + R.getOffset() for the pointer and you don't need the FileManager, the SourceManager, createInMemoryFile(), the PresumedLoc, translateFileLineCol.

You can either try to use SourceMgr::getLineAndColumn() or compute line/col yourself -- either should be a lot less code than the current machinery here.

================
Comment at: clang/tools/clang-format/ClangFormat.cpp:334
+      SMDiagnostic Diags(
+          llvm::SourceMgr(), SMLoc::getFromPointer(StartBuf), AssumedFileName,
+          PLoc.getLine(), PLoc.getColumn(),
----------------
I think the idea is that you have a llvm::SourceMgr() hanging around (above the loop or somewhere). SMDiagnostic stores a pointer to this argument, and you pass in a temporary. The SourceMgr needs to outlive the Diags object. (In practice, the pointer is not used by anything so it happens to work, but having the object store a stale pointer seems like potential future trouble, and it's easy to prevent -- just put the SourceMgr on the stack).

Also, probably "Diag", not "Diags"?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69854/new/

https://reviews.llvm.org/D69854