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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy marked an inline comment as done and 2 inline comments as not done.
vedgy added inline comments.


================
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);
+
----------------
vedgy wrote:
> 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.
A copy of user-provided temporary directory path buffer can be stored in `class CIndexer`. But this API in //llvm/Support/Path.h// would stay fragile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774