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

[Igor Kushnir via Phabricator via llvm-commits]

vedgy added a comment.

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

> This feels like a configuration property of the libclang execution -- so it'd be nice to require it to be set up from `clang_createIndex()` (IIRC that's the entrypoint for CIndex functionality, but I'm not 100% sure), rather than an API that the user can call repeatedly.

In order to keep backward compatibility, this would require introducing another `createIndex` function, e.g. `clang_createIndexWithTempDir()`. `clang_disposeIndex()` would have to un-override the temporary directory then. I'll have to check whether this un-overriding happens too early (before all //preamble-*.pch// files are removed). But notice that separate setup functions already exist: `clang_CXIndex_setGlobalOptions()`, `clang_CXIndex_setInvocationEmissionPathOption()`. The documentation for `clang_setTemporaryDirectory()` can recommend calling it before `clang_createIndex()`.
Either alternative works for KDevelop, because it calls `clang_createIndex()` once.



================
Comment at: clang/include/clang-c/Index.h:78-79
+ * this function in order to reset the temporary directory to the default value
+ * from the environment. Such a resetting should be done before deleting a
+ * tempDirUtf8 pointer previously passed to this function.
+ */
----------------
aaron.ballman wrote:
> Should we mention anything about relative vs absolute path requirements? Separators? 
On Windows separators are converted automatically. I suppose we don't need to warn users not to pass Windows-style separators on Unix.

On Windows this function handles both relative and absolute paths. On Unix the existing implementation doesn't check whether the path is absolute or relative. Perhaps it assumes that the path in the environment variable is always absolute. Or absolute vs relative doesn't matter. I'll check what happens if a relative path is passed.


================
Comment at: llvm/include/llvm/Support/Path.h:423
+/// tempDirUtf8 pointer previously passed to this function.
+void set_system_temp_directory_erased_on_reboot(const char *tempDirUtf8);
+
----------------
aaron.ballman wrote:
> Err, I'm not super excited about this new API. For starters, it's not setting the system temp directory at all (it doesn't make any modifications to the host system); instead it overrides the system temp directory. But also, this is pretty fragile due to allowing the user to override the temp directory after the compiler has already queried for the system temp directory, so now you get files in two different places. Further, it's fragile because the caller is responsible for keeping that pointer valid for the lifetime of the program. Finally, we don't allow you to override any other system directory that you can query (like the home directory).
>  it's not setting the system temp directory at all (it doesn't make any modifications to the host system); instead it overrides the system temp directory.
Rename to `override_system_temp_directory_erased_on_reboot()`?

> this is pretty fragile due to allowing the user to override the temp directory after the compiler has already queried for the system temp directory, so now you get files in two different places.
Unfortunately, I don't know how to prevent this. In practice calling `clang_setTemporaryDirectory()` before `clang_createIndex()` works perfectly well in KDevelop: the //preamble-*.pch// files are created later and never end up in the default temporary directory ///tmp//. The same issue applies to the current querying of the environment variable values repeatedly: the user can set environment variables at any time.

>  it's fragile because the caller is responsible for keeping that pointer valid for the lifetime of the program.
I'd love to duplicate the temporary directory path buffer in Path.cpp. The API would become much easier to use. But then the buffer would have to be destroyed when libclang is unloaded or on program shutdown, which is forbidden by LLVM Coding Standards: //Do not use Static Constructors//. That is, I think the following code in Path.cpp is not acceptable:
```
static SmallVectorImpl<char> &tempDirErasedOnRebootUtf8()
{
  static SmallVector<char> value;
  return value;
}
```

> we don't allow you to override any other system directory that you can query (like the home directory).
Well, one has to start somewhere :) Seriously, overriding directories should be allowed only if it has a practical use case. Not just because overriding others is allowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774