dblaikie added a comment. In D139774#4044258 <https://reviews.llvm.org/D139774#4044258>, @aaron.ballman wrote:
> In D139774#4043886 <https://reviews.llvm.org/D139774#4043886>, @vedgy wrote: > >> In D139774#4041308 <https://reviews.llvm.org/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 mind? >>> >>> We should certainly investigate whether there are other uses of temp files >>> from libclang as part of these changes. It's possible we'll find a >>> situation that makes my suggestion untenable because we don't have easy >>> access to the information we need. >> >> The temporary directory could be used, now or in future, by some support >> code, which is used indirectly by libclang. I found the following uses >> potentially relevant to libclang: >> >> - `Driver::GetTemporaryDirectory(StringRef Prefix)` calls >> `llvm::sys::fs::createUniqueDirectory(Prefix, Path);` >> - `StandardInstrumentations` => `DotCfgChangeReporter` calls >> `sys::fs::createUniquePath("cfgdot-%%%%%%.dot", SV, true);` >> - There are plenty of `createTemporaryFile()` uses in llvm and clang. Some >> of them are likely used by libclang. >> >> I don't know how to efficiently check whether or not each of these indirect >> `system_temp_directory()` uses is in turn used by libclang. Even if they >> aren't know, they might be later, when libclang API grows. > > Oof, that is more exposure than I was thinking we had... > >> On a different note, do you think overriding temporary directory path is >> useful only to libclang and not likely to be useful to any other LLVM API >> users? > > I don't think there are any in-tree needs for that functionality, and the > solution is fragile enough that I'd like to avoid it if possible (for > example, it also makes the API much harder to use across threads because now > it's accessing global state). That said, I think the libclang use case you > have is compelling and worth solving in-tree if we can (so others don't have > to find similar workarounds). > >>>> Another issue is with the `FileSystemOptions` part: this class is widely >>>> used in Clang. So adding a member to it could adversely affect performance >>>> of unrelated code. >>> >>> It's good to keep an eye on that, but I'm not too worried about the >>> overhead from it (I don't see uses in AST nodes). We can keep an eye on >>> https://llvm-compile-time-tracker.com/ to see if there is a surprising >>> compile time increase though. >> >> [in case we pursue this design approach, which I currently don't feel is >> right] Why not add a temporary path data member into `class ASTUnit` under >> the `FileSystemOptions FileSystemOpts` member, not inside it? So that other >> code is not burdened with unused struct member, and to be able to use a more >> suitable type, like `SmallString` for the temporary path buffer. > > That's certainly an option, but the design of that would be a bit strange in > that we have some file system options in one place and other file system > options in another place. I think ultimately, we're going to want them all to > live on `FileSystemOptions`. That said, I'm going to rope in some more folks > for a wider selection of opinions on how/if to proceed. CC @dblaikie @MaskRay > @d0k Yeah, I think, at a quick glance, I'd lean towards FilesystemOptions too - it's mostly used as a member of FileManager, which already has a lot of members/fairly large footprint, so I wouldn't worry too much about this having a huge impact on the total memory usage/perf/etc. I'm not sure how I feel about the general premise (though I don't have enough context/ownership to want to veto any direction/progress here, just thoughts in case they resonate): 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 the like - I think we have such a filesystem abstraction, maybe that only works on Windows? I'm not sure) 2. If there isn't a better general way to avoid creating temp trash that's a problem, I'd have thought that KDevelop/other tools would have issues with other temp files too, and want a more general solution (I'd have thought changing the process's temp directory, and changing it back for user process launches, would be sufficient - so it can cleanup after itself for all files, not just these particular clang-created ones) 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
