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

[Igor Kushnir via Phabricator via llvm-commits]

vedgy marked an inline comment as not done.
vedgy added a comment.

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

> In terms of the C API, I think it'd make more sense to name in terms of "override" rather than "set", but I don't feel as strongly about it given the other setters. In terms of the C++ file system API, I think "override" makes the most sense though (we don't have setters to follow the naming convention for) because some systems do allow you to set the system directory.

Let's keep the naming in C and C++ APIs consistent: `clang_overrideTemporaryDirectory()` and `override_system_temp_directory_erased_on_reboot()`.

> In terms of memory ownership, WDYT of requiring the caller to handle this? e.g., calling `set_system_temp_directory_erased_on_reboot` will `strdup` a nonnull pointer and `free` the stored pointer when given nullptr.

I like this idea. libclang-user code would become easier to use than it is now (though less easy compared to libclang managing memory itself). The libclang API documentation can require overriding the temp directory before creating an index and un-overriding it with `nullptr` after calling `clang_disposeIndex()`.
Now in order to make this libclang API harder to misuse, I lean towards passing the temporary directory in `clang_createIndexWithTempDir()` and letting `clang_disposeIndex()` handle the un-overriding (call `override_system_temp_directory_erased_on_reboot(nullptr)`) automatically. Makes sense? I feel that the `clang_createIndexWithTempDir()` name could be improved, but don't know how...

What memory [de]allocation method should `override_system_temp_directory_erased_on_reboot()` use? `new[]` and `delete[]`? Or should `strdup()` from POSIX be used because it is defined as `_strdup` on Windows? (along with standard `free()`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774