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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4175628 <https://reviews.llvm.org/D143418#4175628>, @aaron.ballman wrote:

> 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 acquire/release semantics is needed if the atomic variable guards access to another non-atomic variable or the atomic pointer guards access to its non-atomic pointed-to value. The relaxed order means no guarantees about this variable's interaction with other atomic or non-atomic variables. But even the relaxed order prevents data races on the variable itself, which is sufficient here.

>> 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.

I also thought about the deprecated old interfaces today. `clang_CXIndex_setGlobalOptions()` could be undeprecated by similarly making `CIndexer::Options` atomic. Safely setting `std::string` members would require a mutex.

> 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?

This would allow to specify whether to store each individual preamble in memory, which is more specific than necessary for my use case. This would shift the burden of passing around the `StorePreamblesInMemory` value to libclang users. Certainly more difficult to implement both in libclang and in KDevelop.

Advantages of getting rid of the option state setters entirely and passing options to per-TU APIs:

1. More flexibility (per-TU option specification).
2. Possibly more efficient (no need for atomics and mutexes).
3. Clearer to API users which TUs the modified options apply to and which TUs (created earlier) keep the original options.
4. Less risk of inconsistencies and bugs in libclang. Such as somehow passing a new option value to an old TU with unpredictable consequences.

Do the listed advantages explain your preference?

I am not sure what I would prefer from a hypothetical standpoint of a libclang maintainer. And you definitely have more experience in this area. So I won't argue for the index option state setters.

I'll probably implement the uncontroversial `CXIndexOptions` member first. And then, if I think implementing it is worth the effort, continue discussing `clang_parseTranslationUnitWithOptions()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418