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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.


================
Comment at: clang/include/clang-c/Index.h:329
+ * CXIndexOptions Opts = { sizeof(CXIndexOptions),
+ *                         clang_getDefaultGlobalOptions() };
+ * \endcode
----------------
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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418