Use TempFile in the implementation of LockFileManager
Needs ReviewPublic

Authored by rafael on Mon, Nov 13, 4:50 PM.

Details

Summary

This move some of the complexity over to the lower level TempFile.

It also makes it a bit more explicit where errors are ignored since we now have a call to consumeError.

Diff Detail

rafael created this revision.Mon, Nov 13, 4:50 PM
benlangmuir requested changes to this revision.Tue, Nov 14, 11:18 AM
benlangmuir added a subscriber: bruno.

Spotted one issue, but otherwise seems okay to me. I'd like @bruno to take a look at this, because he's thought about this code much more recently than me.

lib/Support/LockFileManager.cpp
128

The constructor is being called with a temporary lambda, so this is going to create a use-after-free.

This revision now requires changes to proceed.Tue, Nov 14, 11:18 AM
rafael updated this revision to Diff 122912.Tue, Nov 14, 2:10 PM

Use std::function instead of function_ref.

Ping. This is now the last use of this sys::fs::createUniqueFile variant in llvm proper.

rnk accepted this revision.Fri, Nov 17, 11:40 AM

lgtm, let's give it a go.

lib/Support/LockFileManager.cpp
175

This is total bikeshedding, but I like to write these RAII helpers inline, like:

struct TempFileRemover {
  TempFile &TF;
  LockFileManager &LFM;
  bool Cancelled = false;
  TempFileRemover(TempFile &TF, LockFileManager &LFM) : TF(TF), LFM(LFM) {}
  ~TempFileRemover() {
    if (Cancelled)
      return;
    if (Error E = TF.discard())
      LFM.setError(errorToErrorCode(std::move(E)));
} RemoveTempFile(*TempFile, *this);

It saves a std::function, but you have to write out the capture members and a constructor to initialize them. Maybe this is just what happens when your brain has been damaged from working on a C++ compiler too long.

r318550

I kept the RAII class outside the function since a cancelable cleanup seems a general concept and we might want to move it to a header in the future.

bruno accepted this revision.Fri, Nov 17, 6:33 PM

LGTM as well!