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

Fri Mar 3 08:08:45 PST 2023

aaron.ballman added inline comments.

================
Comment at: clang/include/clang-c/Index.h:329
+ * CXIndexOptions Opts = { sizeof(CXIndexOptions),
+ *                         clang_getDefaultGlobalOptions() };
+ * \endcode
----------------
vedgy wrote:
> aaron.ballman wrote:
> > vedgy wrote:
> > > vedgy wrote:
> > > > aaron.ballman wrote:
> > > > > vedgy wrote:
> > > > > > When I almost finished the requested changes, I remembered that the return value of `clang_getDefaultGlobalOptions()` depends on environment variables, and thus `0` is not necessarily the default. Adjusted the changes and updated this revision.
> > > > > > 
> > > > > > Does the extra requirement to non-zero initialize this second member sway your opinion on the usefulness of the helper function `inline CXIndexOptions clang_getDefaultIndexOptions()`? Note that there may be same (environment) or other important reasons why future new options couldn't be zeroes by default.
> > > > > Thinking out loud a bit... (potentially bad idea incoming)
> > > > > 
> > > > > What if we dropped `clang_getDefaultGlobalOptions()` and instead made a change to `CXGlobalOptFlags`:
> > > > > ```
> > > > > typedef enum {
> > > > >   /**
> > > > >    * Used to indicate that the default CXIndex options are used. By default, no
> > > > >    * global options will be used. However, environment variables may change which
> > > > >    * global options are in effect at runtime.
> > > > >    */
> > > > >   CXGlobalOpt_Default = 0x0,
> > > > > 
> > > > >   /**
> > > > >    * Used to indicate that threads that libclang creates for indexing
> > > > >    * purposes should use background priority.
> > > > >    *
> > > > >    * Affects #clang_indexSourceFile, #clang_indexTranslationUnit,
> > > > >    * #clang_parseTranslationUnit, #clang_saveTranslationUnit.
> > > > >    */
> > > > >   CXGlobalOpt_ThreadBackgroundPriorityForIndexing = 0x1,
> > > > > 
> > > > >   /**
> > > > >    * Used to indicate that threads that libclang creates for editing
> > > > >    * purposes should use background priority.
> > > > >    *
> > > > >    * Affects #clang_reparseTranslationUnit, #clang_codeCompleteAt,
> > > > >    * #clang_annotateTokens
> > > > >    */
> > > > >   CXGlobalOpt_ThreadBackgroundPriorityForEditing = 0x2,
> > > > > 
> > > > >   /**
> > > > >    * Used to indicate that all threads that libclang creates should use
> > > > >    * background priority.
> > > > >    */
> > > > >   CXGlobalOpt_ThreadBackgroundPriorityForAll =
> > > > >       CXGlobalOpt_ThreadBackgroundPriorityForIndexing |
> > > > >       CXGlobalOpt_ThreadBackgroundPriorityForEditing,
> > > > > 
> > > > >   /**
> > > > >    * Used to indicate that no global options should be used, even
> > > > >    * in the presence of environment variables.
> > > > >    */
> > > > >   CXGlobalOpt_None = 0xFFFFFFFF
> > > > > } CXGlobalOptFlags;
> > > > > ```
> > > > > so that when the user passes `0` they get the previous behavior.
> > > > > 
> > > > > `clang_CXIndex_setGlobalOptions()` would remain deprecated. `clang_CXIndex_getGlobalOptions()` would be interesting though -- would it return `CXGlobalOpt_None` or `CXGlobalOpt_Default` in the event the index was created without any global options? Hmmm.
> > > > > 
> > > > > Err, actually, I suppose this won't work too well because then code silently changes behavior if it does `clang_CXIndex_setGlobalOptions(CXGlobalOpt_None);` because that would change from "do what the environment says" to "ignore the environment". But I have to wonder whether anyone actually *does* that or not... my intuition is that folks would not call `clang_CXIndex_setGlobalOptions()` at all unless they were setting an option to a non-none value. We could rename `CXGlobalOpt_None` to `CXGlobalOpt_Nothing` (or something along those lines) to force a compilation error, but that's a bit of a nuclear option for what's supposed to be a stable API.
> > > > > 
> > > > > So I'm on the fence, I guess. I'd still prefer for zero to give sensible defaults and I don't think there's enough use of the global options + environment variables to matter. But I also don't like silently breaking code, so my idea above may be a nonstarter.
> > > > > 
> > > > > I suppose another possible idea is: deprecate the notion of global options enum and setter/getter entirely, add two new fields to `CXIndexOptions`:
> > > > > ```
> > > > > typedef enum {
> > > > >   CXChoice_Default = 0,
> > > > >   CXChoice_Enabled = 1,
> > > > >   CXChoice_Disabled = 2
> > > > > } CXChoice;
> > > > > 
> > > > > ...
> > > > > unsigned ThreadPriorityBackgroundForIndexing;
> > > > > unsigned ThreadPriorityBackgroundForEditing;
> > > > > ...
> > > > > ```
> > > > > so that `0` gives the correct default behavior based on environment variable. There would be no global setter or getter for this information (and we'd eventually remove `clang_CXIndex_[gs]etGlobalOptions()`).
> > > > > I suppose this won't work too well because then code silently changes behavior if it does `clang_CXIndex_setGlobalOptions(CXGlobalOpt_None);` because that would change from "do what the environment says" to "ignore the environment".
> > > > No, the current consequence of such a call already is to ignore the environment. What would change is the consequence of calling `clang_CXIndex_setGlobalOptions(0);` - from "ignore the environment" to "do what the environment says".
> > > > 
> > > > > But I have to wonder whether anyone actually *does* that or not... my intuition is that folks would not call `clang_CXIndex_setGlobalOptions()` at all unless they were setting an option to a non-none value.
> > > > I agree. Possible unlikely reasons to call `clang_CXIndex_setGlobalOptions(0)` are:
> > > > 1) in case the environment variables are set for some other program;
> > > > 2) in case setting the environment variables had been useful in the past but not in the latest and greatest version of an IDE;
> > > > 3) if background priority is never useful for an IDE.
> > > > 
> > > > > I suppose another possible idea is: deprecate the notion of global options enum and setter/getter entirely, add two new fields to CXIndexOptions
> > > > This appears to be a great idea to me. The notion of `CXGlobalOptFlags` somewhat conflicts with the new `CXIndexOptions` struct, in which two other boolean options are represented by bit-fields.
> > > > 
> > > > I think we can forbid from the start calling `clang_CXIndex_[gs]etGlobalOptions()` if the index is created via the new function `clang_createIndexWithOptions`.
> > > > 
> > > > If 3-state environment variables (unspecified/on/off) are introduced in the future, `CXChoice` could be extended with `CXChoice_FromEnvironmentOrEnabled = 3` to indicate that if the environment variable is present, its value should be respected, otherwise the thread priority should be enabled.
> > > > 
> > > > `CXChoice` cannot possibly have many valid values. So how about:
> > > > ```
> > > > unsigned char ThreadPriorityBackgroundForIndexing;
> > > > unsigned char ThreadPriorityBackgroundForEditing;
> > > > ```
> > > > Then `size_t Size` could become `unsigned Size` and all non-pointer options would fit into 8 bytes on x86_64.
> > > > 
> > > > Did you reorder the words in the variable names intentionally? `CXGlobalOpt_ThreadBackgroundPriorityForIndexing` => `ThreadPriorityBackgroundForIndexing`
> > > Perhaps `clang_CXIndex_getGlobalOptions()` should not be deprecated. It would allow the application to learn which priorities are actually used by libclang.
> > > I think we can forbid from the start calling clang_CXIndex_[gs]etGlobalOptions() if the index is created via the new function clang_createIndexWithOptions.
> > 
> > I think that would be great, at least for the setter. As you mention, we might not want to deprecate the getter (so long as it reports accurate information to the caller) because that could be useful still. However, I was also thinking that same logic could apply (at least in theory) to other options in the structure and so we might want to deprecate it with a replacement that gets all of the options from the index at once. However, that felt like scope creep for this already long-running patch.
> > 
> > > CXChoice cannot possibly have many valid values. So how about:
> > > ```
> > > unsigned char ThreadPriorityBackgroundForIndexing;
> > > unsigned char ThreadPriorityBackgroundForEditing;
> > > ```
> > > Then size_t Size could become unsigned Size and all non-pointer options would fit into 8 bytes on x86_64.
> > 
> > Hmmm that could work, but how about:
> > ```
> > /* Stores a value of type CXChoice. */
> > int ThreadPriorityBackgroundForIndexing : 2;
> > /* Stores a value of type CXChoice. */
> > int ThreadPriorityBackgroundForEditing : 2;
> > ```
> > so that they pack together with the existing bit-fields?
> > 
> > > Did you reorder the words in the variable names intentionally? CXGlobalOpt_ThreadBackgroundPriorityForIndexing => ThreadPriorityBackgroundForIndexing
> > 
> > Nope, sorry for the confusion, that was a think-o on my part.
> > I think that would be great, at least for the setter. As you mention, we might not want to deprecate the getter (so long as it reports accurate information to the caller) because that could be useful still. However, I was also thinking that same logic could apply (at least in theory) to other options in the structure and so we might want to deprecate it with a replacement that gets all of the options from the index at once. However, that felt like scope creep for this already long-running patch.
> For the time being, only the two global options depend on the environment. Though one could get the system temporary directory path if `PreambleStoragePath` is not overridden. Anyway, there is no known use case for getting such information, so no need to implement this right now, especially since `clang_CXIndex_getGlobalOptions()` already exists and would work correctly without modifications.
> 
> > > CXChoice cannot possibly have many valid values. So how about:
> > > ```
> > > unsigned char ThreadPriorityBackgroundForIndexing;
> > > unsigned char ThreadPriorityBackgroundForEditing;
> > > ```
> > > Then size_t Size could become unsigned Size and all non-pointer options would fit into 8 bytes on x86_64.
> > 
> > Hmmm that could work, but how about:
> > ```
> > /* Stores a value of type CXChoice. */
> > int ThreadPriorityBackgroundForIndexing : 2;
> > /* Stores a value of type CXChoice. */
> > int ThreadPriorityBackgroundForEditing : 2;
> > ```
> > so that they pack together with the existing bit-fields?
> The tighter packing wouldn't reduce the size of the struct further in LLVM 17, because most likely only one more boolean (`StorePreamblesInMemory`) will be added to the struct in this release. In future releases the struct size would have to increase for versioning purposes anyway. But the 2-bitness //would// reduce the flexibility of `CXChoice`. I have already invented a 4th possible enumerator. Someone could come up with a fifth in the future. So I'd prefer `unsigned char` or at least `: 4` (4 bits) for these non-boolean options.
> The tighter packing wouldn't reduce the size of the struct further in LLVM 17, because most likely only one more boolean (StorePreamblesInMemory) will be added to the struct in this release. 

Okay, fair point, let's stick with `unsigned char` then (tbh, I don't know that I'd be upset if it was `int` either -- this structure is passed by pointer anyway).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418