[PATCH] D143418: [libclang] Add API to override preamble storage path

Tue Mar 7 09:03:13 PST 2023

aaron.ballman added a comment.

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

> I am implementing the `StorePreamblesInMemory` bool option discussed earlier.  It would be nice to be able to modify it at any time, because it can be an option in an IDE's UI and requiring to restart an IDE for the option change to take effect is undesirable. In order to make the setter thread-safe, the option can be stored as `std::atomic<bool> StorePreamblesInMemory` in `class CIndexer` and stored/loaded with `memory_order_relaxed`. Setting this option would apply only to subsequent `clang_parseTranslationUnit_Impl()` calls.

Hmmm, don't relaxed loads and stores still have the potential to be racey? I thought you needed a release on the store and an acquire on the load (or sequential consistency), but this is definitely not my area of expertise.

> The option can also be added to `CXIndexOptions` in order to allow setting its initial value reliably (not worrying whether it could be used before the setter gets called after index construction).
>
> Is adding both the setter and the `CXIndexOptions` member OK or would you prefer only one of these two?

It's a bit odd to me that we're deprecating the global option setters to turn around and add a new global option setter. The old interfaces are not thread safe and the new one is thread safe, so I get why the desire exists and is reasonable, but (personally) I'd like to get rid of the option state setters entirely someday. What I was envisioning was that the index creation would get global options and if we wanted per-TU options, we'd add an interface akin to `clang_parseTranslationUnit()` which takes another options struct for per-TU options. (Perhaps the default values for those options come from the global options -- e.g., `DisplayDiagnostics` is set at the global level but could be overridden for a single TU). Do you have thoughts about that approach?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418