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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
