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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
