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

vedgy abandoned this revision. vedgy added a comment. D143418 implements my latest idea described in the previous comment. Let us continue the discussion there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139774

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

aaron.ballman added a comment. In D139774#4098695 , @vedgy wrote: > Another disadvantage of using `CINDEX_VERSION_MINOR` instead of the `sizeof` > is that an older libclang version won't know about compatibility of newer > versions with itself. But this

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

vedgy added a comment. Another disadvantage of using `CINDEX_VERSION_MINOR` instead of the `sizeof` is that an older libclang version won't know about compatibility of newer versions with itself. But this reasoning brought me to a different thought: when a program is compiled against a libclang

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

aaron.ballman added a comment. In D139774#4096733 , @vedgy wrote: > In D139774#4096695 , @aaron.ballman > wrote: > >> There's three scenarios when a field is added to the structure: 1) library >> and app are mat

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

vedgy added a comment. Perhaps the struct should contain the value of `CINDEX_VERSION_MINOR` instead of the `sizeof`? This way, libclang can change the meaning of a struct member without changing the size of the struct. For example, replace `PreambleStoragePath` with `TemporaryDirectoryPath`. S

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

vedgy added a comment. In D139774#4096695 , @aaron.ballman wrote: > There's three scenarios when a field is added to the structure: 1) library > and app are matching versions, 2) library is newer than app, 3) app is newer > than library. #1 is the happ

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

aaron.ballman added a comment. In D139774#4096638 , @vedgy wrote: > In D139774#4096527 , @aaron.ballman > wrote: > >> That sounds like a good plan to me. I wonder if we want to name it something >> like `clang_c

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

vedgy added a comment. In D139774#4096527 , @aaron.ballman wrote: > That sounds like a good plan to me. I wonder if we want to name it something > like `clang_createIndexWithOptions` (or something generic like that), and > give it a versioned structure

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

aaron.ballman added a comment. In D139774#4095522 , @vedgy wrote: > In D139774#4094619 , @aaron.ballman > wrote: > >> Is there a middle ground where, instead of #2 for general temporary storage, >> we went with

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

vedgy added a comment. In D139774#4094619 , @aaron.ballman wrote: > Is there a middle ground where, instead of #2 for general temporary storage, > we went with #2 but with compiler-specific directories instead of system > directories. e.g., don't let t

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

aaron.ballman added a comment. In D139774#4092553 , @vedgy wrote: > In D139774#4091631 , @aaron.ballman > wrote: > >> My preference is still for specific API names. If we want to do something >> general to all t

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

vedgy added a comment. In D139774#4091631 , @aaron.ballman wrote: > My preference is still for specific API names. If we want to do something > general to all temp files, I think `FileSystemOptions` is the way to go. > > My concern with not using a cons

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

aaron.ballman added a comment. In D139774#4081888 , @vedgy wrote: > 3 out of 4 alternatives remain: > >> 1. Add an option to store the preamble-*.pch files in RAM instead of /tmp >> and add a corresponding option in KDevelop configuration UI. This would

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

vedgy added a comment. 3 out of 4 alternatives remain: > 1. Add an option to store the preamble-*.pch files in RAM instead of /tmp and > add a corresponding option in KDevelop configuration UI. This would work > perfectly for me, provided I don't change my mind and decide to turn this > option

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

aaron.ballman added a comment. In D139774#4079249 , @vedgy wrote: > Now that a consensus has been reached that this revision is to be discarded, > can we please resume the discussion of which alternative should be > implemented in the replacement revisi

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

vedgy added a comment. Now that a consensus has been reached that this revision is to be discarded, can we please resume the discussion of which alternative should be implemented in the replacement revision? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

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

dblaikie added a comment. In D139774#4069014 , @aaron.ballman wrote: > In D139774#4066920 , @dblaikie > wrote: > >> Don't let me hold this up - I think it all feels a bit too ad-hoc for my own >> preferences (f

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

erichkeane added inline comments. Comment at: llvm/lib/Support/Unix/Path.inc:1450 void system_temp_directory(bool ErasedOnReboot, SmallVectorImpl &Result) { Result.clear(); So I was asked to take a look at this, and I believe that changing this function i

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

aaron.ballman added a comment. In D139774#4066920 , @dblaikie wrote: > Don't let me hold this up - I think it all feels a bit too ad-hoc for my own > preferences (feels like there should be fairly general solutions to this - > rather than playing whack-

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

vedgy added a comment. One idea discussed in comments to the KDevelop merge request, which I haven't mentioned here is this: remove the preamble files immediately after creating and opening them. This is safe on Unix-like OSes, because every file that is still open will not actually be deleted

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

dblaikie added a comment. Don't let me hold this up - I think it all feels a bit too ad-hoc for my own preferences (feels like there should be fairly general solutions to this - rather than playing whack-a-mole on only the biggest temporary files - both in terms of the options KDevelop develope

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

vedgy added a comment. In D139774#4066593 , @dblaikie wrote: >>> (1) seems OK-ish, I guess there's some question as to what the tradeoff is >>> for that option - does that blow out memory usage of the client/kdevelop? >>> But I guess it's probably fine.

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

dblaikie added a comment. In D139774#4066574 , @aaron.ballman wrote: > In D139774#4066519 , @dblaikie > wrote: > >> I've mixed feelings about this, but yeah, I guess the issues with global >> state are pretty u

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

aaron.ballman added a comment. In D139774#4066519 , @dblaikie wrote: > I've mixed feelings about this, but yeah, I guess the issues with global > state are pretty undesirable. I don't worry too much about (2) - doesn't feel > too problematic to me (for

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

vedgy added a comment. In D139774#4066519 , @dblaikie wrote: > (1) seems OK-ish, I guess there's some question as to what the tradeoff is > for that option - does that blow out memory usage of the client/kdevelop? But > I guess it's probably fine. At l

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

dblaikie added a comment. In D139774#4065753 , @aaron.ballman wrote: > In D139774#4063131 , @vedgy wrote: > >> After a discussion under the corresponding KDevelop merge request, I can see >> 4-6 alternative ways

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

vedgy added a comment. In D139774#4065753 , @aaron.ballman wrote: > I lean towards #2b over #2a due to wanting individual overrides rather than a > blanket override (e.g., we should be able to put preamble files in a > different location than we put, s

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

aaron.ballman added a comment. In D139774#4063131 , @vedgy wrote: > After a discussion under the corresponding KDevelop merge request, I can see > 4-6 alternative ways to address the temporary directory issue: > > 1. Add an option to store the //preamble

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

vedgy added a comment. After a discussion under the corresponding KDevelop merge request, I can see 4-6 alternative ways to address the temporary directory issue: 1. Add an option to store the //preamble-*.pch// files in RAM instead of /tmp and add a corresponding option in KDevelop configurati

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

vedgy added a comment. In D139774#4047976 , @dblaikie wrote: > It seemed like the places where kdevelop would want to suppress the temp dir > cleanup would be enumerable/more within kdevelop's control than instances of > libraries kdevelop is using want

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

dblaikie added a comment. In D139774#4046308 , @vedgy wrote: > In D139774#4045361 , @dblaikie > wrote: > >> 1. Should clang be doing something better with these temp files anyway, no >> matter the directory they

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

vedgy added a subscriber: milianw. vedgy added a comment. In D139774#4045361 , @dblaikie wrote: > 1. Should clang be doing something better with these temp files anyway, no > matter the directory they go in? (setting them for cleanup at process exit or

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

MaskRay added a comment. libclang functions like `clang_reparseTranslationUnit_Impl` call `clang/lib/Frontend/ASTUnit.cpp:1397` and build the preamble with `/*StoreInMemory=*/false`. If `StoreInMemory` is configurable (true), then you can avoid the temporary file `preamble-*.pch`. Repository:

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

dblaikie added a comment. In D139774#4044258 , @aaron.ballman wrote: > In D139774#4043886 , @vedgy wrote: > >> In D139774#4041308 , >> @aaron.ballman wrote: >> >>> Is yo

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

aaron.ballman added subscribers: MaskRay, dblaikie. aaron.ballman added a comment. In D139774#4043886 , @vedgy wrote: > In D139774#4041308 , @aaron.ballman > wrote: > >> Is your concern essentially that someone w

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

vedgy added a comment. In D139774#4041308 , @aaron.ballman wrote: > Is your concern essentially that someone will add a new use to put files in a > temp directory and not have access to this information from ASTUnit? Or do > you have other concerns in

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

aaron.ballman added a comment. In D139774#4041169 , @vedgy wrote: > In D139774#4040705 , @aaron.ballman > wrote: > >> At a point where we have a `CIndexer` object, we eventually call >> `ASTUnit::LoadFromCommand

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

vedgy added a comment. In D139774#4040705 , @aaron.ballman wrote: > At a point where we have a `CIndexer` object, we eventually call > `ASTUnit::LoadFromCommandLine()` and `ASTUnit` has a member > `FileSystemOptions FileSystemOpts`, and `FileSystemOpti

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

aaron.ballman added a comment. In D139774#4040057 , @vedgy wrote: > In D139774#4039975 , @aaron.ballman > wrote: > >> Oh that is a good point! Apologies for not noticing that earlier -- that >> makes me wonder i

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

vedgy added a comment. In D139774#4039975 , @aaron.ballman wrote: > Oh that is a good point! Apologies for not noticing that earlier -- that > makes me wonder if a better approach here is to add a `std::string` to the > `CIndexer` class to represent th

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

aaron.ballman added a comment. In D139774#4039018 , @vedgy wrote: > In D139774#4036496 , @aaron.ballman > wrote: > >> Given that we already have other setters to set global state, I'm less >> concerned about add

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

vedgy added a comment. In D139774#4036496 , @aaron.ballman wrote: > Given that we already have other setters to set global state, I'm less > concerned about adding another one. I hadn't realized we already introduced > the dangers here. But we should d

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

vedgy marked an inline comment as not done. vedgy added a comment. In 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

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

aaron.ballman added a comment. Given that we already have other setters to set global state, I'm less concerned about adding another one. I hadn't realized we already introduced the dangers here. But we should document the expectation that the call be made before creating the index. In terms o

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

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 *tempDi

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

vedgy added a comment. In D139774#4025356 , @vedgy wrote: > `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 remov

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

vedgy added a comment. In 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 f

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

aaron.ballman added a comment. I'm sympathetic to the problem you're trying to solve (not having to set environment variable for the temp directory), but I'm also not convinced this is the correct way to approach it. This feels like a configuration property of the libclang execution -- so it'd

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

vedgy updated this revision to Diff 481913. vedgy edited the summary of this revision. vedgy added a comment. Extract identical code from the two Path.inc files into Path.cpp One of the Path.inc files is #include-d into this Path.cpp file and nowhere else. Repository: rG LLVM Github Monorepo

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

vedgy created this revision. vedgy added a reviewer: aaron.ballman. Herald added subscribers: arphaman, hiraditya. Herald added a project: All. vedgy requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Fixes: https://github.co