Page MenuHomePhabricator

Support `ivfsoverlay` option in Tooling
AbandonedPublic

Authored by vladimir.plyashkun on Dec 27 2017, 5:53 AM.

Details

Summary

Previously, this argument had no effect, since it didn't proceeded.
For more information, check this review: https://reviews.llvm.org/D41535

Diff Detail

Repository
rC Clang

Event Timeline

I'm not sure if it's ok to ignore the shared FileManager here.
@klimek, @bkramer, @alexfh, does tooling library rely on having the same FileManager for all invocations? Is it just a performance optimization or there's more to it?

lib/Tooling/Tooling.cpp
287

Files is a raw pointer, so we're leaking memory here.

lib/Tooling/Tooling.cpp
287

Agree.
I can try to replace type of this field to llvm::IntrusiveRefCntPtr<T> instead of raw pointer.
But if i understand correctly, this class is available during whole execution, so memory will be freed on exit.
I saw some other usages, for example, createFileManager method in the CompilerInstance also just reassign value to raw pointer.
https://clang.llvm.org/doxygen/classclang_1_1CompilerInstance.html#abeb2bbf46a8de987c227125a84935802

ilya-biryukov added inline comments.Dec 28 2017, 2:51 AM
lib/Tooling/Tooling.cpp
287

Using IntrusiveRefCntPtr<FileManager> locally should do the trick, the clients can take ownership if they want and FileManager will be properly freed if they don't do that.

CompilerInstance::createFileManager stores IntrusiveRefCntPtr as a field before returning a raw pointer, so it seems to properly manage memory there.

With D41947 in place, do we still need this change? Or can it be "abandoned" now?