[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

3 out of 4 alternatives remain:

> 1. Add an option to store the preamble-*.pch files in RAM instead of /tmp and add a corresponding option in KDevelop configuration UI. This would work perfectly for me, provided I don't change my mind and decide to turn this option off, in which case I'll be back to square one.
> 2. Add an option to store the preamble-*.pch files in a custom directory instead of a general temporary directory. The option could be named generally (2a: overrideTemporaryDirectory) or specially (2b: setPreambleStoragePath). If the option is named generally, other temporary files created by libclang could be identified in the future and placed in the same directory without changing the API.
> 3. 1 and 2 - the options can be propagated between identical end points together, so this combination is natural.

I think **#3** is the way to go. @aaron.ballman has agreed to this.

**#3a** vs **#3b** hasn't been decided upon yet:

> The bool (1) and the path (2) options can be passed through API layers together in a struct. They can both be named generally (preferStoringTempFilesInMemory, setTemporaryDirectory) or specifically (storePreamblesInMemory, setPreambleStoragePath).

@aaron.ballman has put forth arguments in favor of specific names and separate API for different temporary directory uses. I have replied with counterarguments in favor of general temporary storage API.

In D139774#4081055 <https://reviews.llvm.org/D139774#4081055>, @aaron.ballman wrote:

> modify `FileSystemOptions` to store an override for the temp directory

I have mentioned that `FileSystemOptions` is widely used and only class `ASTUnit` would need the temp directory (and possibly a flag whether to prefer RAM storage). The wide usage in itself is not considered a reason not to add data members.

`ASTUnit::FileSystemOpts` is used in several places in //ASTUnit.cpp//:

  FileSystemOpts = Clang->getFileSystemOpts();
  AST->FileSystemOpts = CI->getFileSystemOpts();
  AST->FileMgr = new FileManager(AST->FileSystemOpts, VFS);
  AST->FileSystemOpts = FileMgr->getFileSystemOpts();

I don't know whether propagating the new options to and from `CompilerInstance`, `CompilerInvocation` and `FileManager` is a good idea. As of now, only `ASTUnit::getMainBufferWithPrecompiledPreamble()` is going to use the new options.

> and updating APIs so that the temp directory can be specified by a new CIndex constructor function.

Now that the temporary directory options are going to be propagated explicitly, I am no longer sure another constructor function is preferable to separate option setter(s). I don't see any use for temporary directory location in the definition of `clang_createIndex()`. And modifying the temporary directory location multiple times during program execution is no longer a problem: an updated location will be used for new preambles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774