vedgy marked an inline comment as not done.
vedgy 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:
> > > 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.
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
