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

[Igor Kushnir via Phabricator via llvm-commits]

vedgy added a comment.

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

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

The temporary directory could be used, now or in future, by some support code, which is used indirectly by libclang. I found the following uses potentially relevant to libclang:

- `Driver::GetTemporaryDirectory(StringRef Prefix)` calls `llvm::sys::fs::createUniqueDirectory(Prefix, Path);`
- `StandardInstrumentations` => `DotCfgChangeReporter` calls `sys::fs::createUniquePath("cfgdot-%%%%%%.dot", SV, true);`
- There are plenty of `createTemporaryFile()` uses in llvm and clang. Some of them are likely used by libclang.

I don't know how to efficiently check whether or not each of these indirect `system_temp_directory()` uses is in turn used by libclang. Even if they aren't know, they might be later, when libclang API grows.

On a different note, do you think overriding temporary directory path is useful only to libclang and not likely to be useful to any other LLVM API users?

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

[in case we pursue this design approach, which I currently don't feel is right] Why not add a temporary path data member into `class ASTUnit` under the `FileSystemOptions FileSystemOpts` member, not inside it? So that other code is not burdened with unused struct member, and to be able to use a more suitable type, like `SmallString` for the temporary path buffer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774