aaron.ballman added inline comments.
================
Comment at: clang/include/clang-c/Index.h:329
+ * CXIndexOptions Opts = { sizeof(CXIndexOptions),
+ * clang_getDefaultGlobalOptions() };
+ * \endcode
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143418/new/
https://reviews.llvm.org/D143418
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
