[PATCH] D143418: [libclang] Add API to set preferred temp dir path

Thu Feb 9 13:00:27 PST 2023

aaron.ballman added a subscriber: jkorous.
aaron.ballman added a comment.

In D143418#4111990 <https://reviews.llvm.org/D143418#4111990>, @vedgy wrote:

> I hoped to avoid applying an unrelated `git clang-format`'s include reordering, but the CI fails because of that. Will apply it along with changes requested during code review.

FWIW, you can ignore any CI failure related to formatting. It's an (annoying) bug that it gets reported as a failure. Our coding style guide prefers matching the surrounding style when it conflicts with the usual coding style, so it's not uncommon to have these kind of fake failures.

>From the previous review (https://reviews.llvm.org/D139774):

> After thinking some more, I'd like to revisit this decision. You agree that the general solution setTempDirPath() is an ultimate goal. But the rejection of tampering with the return value of system_temp_directory() means that overriding the temporary path will likely never be perfectly reliable.

Just to make sure we're on the same page, the reason we don't want to change `system_temp_directory()` is because that API is expected to get you the system temp directory, so an override in that function would break expectations. Instead, we want callers of that API to decide whether they really want the system temp directory or whether they want some other directory that perhaps the user provides. That keeps a clean separation of concerns -- the system APIs get you the system information and the things that wish to put files elsewhere can still do so.

> So how about a more sincere general solution: setPreferredTempDirPath()? The documentation would say that libclang tries its best to place temporary files into the specified directory, but some might end up in the system temporary directory instead.

I don't think those semantics are reasonable. "Tries its best" is fine for things that are heuristic based, like deciding when to do an analysis cut point, but is too non-deterministic for something like "where do files get placed". I think it's reasonable that we either:

1. expose an option to say "I prefer preambles to be put in this directory" for libclang, or
2. expose an option to say "I prefer using this directory over the temp directory for all files" for libclang,

I thought we agreed that #2 is too high of risk because it requires auditing libclang for all the places it decides to put something into the temp directory, and so we were going with #1 and doing it as part of the cindex constructor so that users cannot run into the surprise issues (multithreading, files changing location mid-run) with an independent setter.

> Such an API would be consistent with an existing libclang function:

That is interesting to note, but I'm not certain that was a good decision as it still has the same concerns. In the original review for that (https://reviews.llvm.org/D40527), it was observed that libclang is not thread-safe, which is accurate (Clang itself is not particularly thread-safe but there have been occasional efforts to drive us in that direction or at least not drive us away from it), but it didn't discuss the fact that this design potentially leads to files in different locations mid-run. It also didn't get nearly as much review discussion as this API, so there's that. CC @arphaman and @jkorous in case they want to weigh in on the new API, since they've worked in this area.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418