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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D139774#4081888 <https://reviews.llvm.org/D139774#4081888>, @vedgy wrote:

> 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.

I can live with it.

> **#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.

My preference is still for specific API names. If we want to do something general to all temp files, I think `FileSystemOptions` is the way to go.

> 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. The store-in-memory bool option in particular should be possible to modify at any time, because it can be an option in an IDE's UI.

My concern with not using a constructor is that, because this is the C API, we will be supporting the functionality for a long time even if we do switch to using a global override in `FileSystemOptions` where you would need to set the information up before executing the compiler. To me, these paths are something that should not be possible to change once the compiler has started executing. (That also solves the multithreading problem where multiple threads are fighting over what the temp directory should be and stepping on each other's toes.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774