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

Tue Jan 31 12:35:37 PST 2023

aaron.ballman added a comment.

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

> In D139774#4091631 <https://reviews.llvm.org/D139774#4091631>, @aaron.ballman wrote:
>
>> 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.
>>
>> 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.)
>
> As I understand, this leaves two choices:
>
> 1. Specific preamble API names, two separate non-constructor setters; the option values are stored in a separate struct (or even as two separate data members), not inside `FileSystemOptions`.
> 2. General temporary-storage arguments (possibly combined in a struct) to a new overloaded constructor function; the option values are stored inside the `FileSystemOptions` struct.
>
> The second alternative is likely more difficult to implement, more risky and less convenient to use (the store-in-memory bool option cannot be modified at any time). Perhaps it should be delayed until (and if) we learn of other temporary files libclang creates? A downside of implementing the first option now is that the specific API would have to be supported for a long time, even after the general temporary-file API is implemented.

I still think the general solution is where we ultimately want to get to and that makes me hesitant to go with specific preamble API names despite that being the direction you prefer. If this wasn't the C APIs, I'd be less concerned because we make no stability guarantees about our C++ interfaces. But we try really hard not to break the C APIs, so adding the specific names is basically committing to not only the APIs but their semantics. I think that makes implementing the general solution slightly more awkward because these are weird special cases that barely get tested in-tree, so they'd be very easy to overlook and accidentally break.

Is there a middle ground where, instead of #2 for general temporary storage, we went with #2 but with compiler-specific directories instead of system directories. e.g., don't let the caller set the temp directory, but do let the caller set the preamble directory (which defaults to the temp directory) so long as it's set before invoking the compiler? This still won't let you change options mid-run, but it also seems like it should have less risk of affecting other components while still solving the thread safety issues. I'm not certain if it's any easier to implement, but I think it does save you from modifying `FileSystemOptions`. As a separate item, we could then consider adding a new C API to let you toggle the in-memory vs on-disk functionality after exploring that it won't cause other problems because nobody considered the possibility that it's not a stable value for the compiler invocation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774