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