dblaikie added a comment. In D139774#4065753 <https://reviews.llvm.org/D139774#4065753>, @aaron.ballman wrote:
> In D139774#4063131 <https://reviews.llvm.org/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-*.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 off, in which case I'll be back to square one. >> 2. Add an option to store the //preamble-*.pch// files in a custom directory >> instead of a general temporary directory. The option could be named >> generally (2a: overrideTemporaryDirectory) or specially (2b: >> setPreambleStoragePath). If the option is named generally, other temporary >> files created by libclang could be identified in the future and placed in >> the same directory without changing the API. >> 3. 1 and 2 - the options can be propagated between identical end points >> together, so this combination is natural. >> 4. The current patch. This is the easiest (already implemented) and most >> reliable (the temporary directory is definitely and completely overridden) >> approach. But there are thread safety concerns due to the introduction of >> global state. >> >> If the 4th option is unacceptable, I lean towards the option 3a (1 and 2a), >> because the amount of implementation work should not be much greater than 1 >> alone or 2a alone. If the 4th option is unacceptable, I suppose a separate >> review request based on the current LLVM main branch should be created and >> this one closed, right? > > From a design perspective, my preference is to not add new APIs at the file > system support layer in LLVM to override the temporary directory but instead > allow individual components to override the decision where to put files. > Overriding a system directory at the LLVM support level feels unclean to me > because 1) threading concerns (mostly related to lock performance), 2) > consistency concerns (put files in temp directory, override temp directory, > put additional files in the new directory), 3) principle of least surprise. > To me, the LLVM layer is responsible for interfacing with the system and > asking for the system temp directory should get you the same answer as > querying for that from the OS directly. 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 any individual file, presumably the implementation recorded the name of the file if they were going to access it again - so still be using the old path if necessary). But, yeah, lack of any "system" abstraction that could be passed into all the various LLVM/MC/AST contexts to swap out the implementation limits the options a bit to more ad-hoc/case-by-case solutions. I feel like that's not a great solution to KDevelop's general problem, though - they're dealing with one particularly big file, but it doesn't feel very principled to me to fix that one compared to all the other (albeit smaller) temp files & maybe at some point in the future some bigger temp files are created elsewhere... > So my preference is for components to have an option to pick a different > location as part of their API layer, rather than at the lower level. Based on > that, I'm definitely in support of #1, and I'm in support of one of the > options for #2. 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, say, crash logs). > > @dblaikie does that sound reasonable to you? (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. 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
