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

Tue Jan 10 13:17:43 PST 2023

aaron.ballman added a comment.

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

> In D139774#4040705 <https://reviews.llvm.org/D139774#4040705>, @aaron.ballman wrote:
>
>> At a point where we have a `CIndexer` object, we eventually call `ASTUnit::LoadFromCommandLine()` and `ASTUnit` has a member `FileSystemOptions FileSystemOpts`, and `FileSystemOptions` has a `std::string` for the working directory to use. I think we should store the temp directory in here as well, and when `ASTUnit::getMainBufferWithPrecompiledPreamble()` build the preamble, it can pass that information along to `TempPCHFile` to avoid needing to ask the system for the temp directory.
>
> The path would have to be passed into several functions.

Understood, and hopefully that doesn't make this a viral slog.

> `TempPCHFile::create()` would have to use `createUniqueFile()` instead of `createTemporaryFile()`. My greatest concern with this improved design is that libclang may use the temporary directory for other purposes - if not yet, then in the future.

Is your concern essentially that someone will add a new use to put files in a temp directory and not have access to this information from ASTUnit? Or do you have other concerns in mind?

We should certainly investigate whether there are other uses of temp files from libclang as part of these changes. It's possible we'll find a situation that makes my suggestion untenable because we don't have easy access to the information we need.

> Another issue is with the `FileSystemOptions` part: this class is widely used in Clang. So adding a member to it could adversely affect performance of unrelated code.

It's good to keep an eye on that, but I'm not too worried about the overhead from it (I don't see uses in AST nodes). We can keep an eye on https://llvm-compile-time-tracker.com/ to see if there is a surprising compile time increase though.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774