aaron.ballman added a comment.
In D143418#4125098 <https://reviews.llvm.org/D143418#4125098>, @vedgy wrote:
>> `uint32_t Size; // = sizeof(struct CIndexOptions), used for option
>> versioning`
>
> 1. `uint32_t` was introduced in C99. Can/should it be used in //Index.h//?
> Only built-in `[unsigned] (int|long)` types are currently used in this file.
That is a really good question. I couldn't spot anything existing in the header
file that requires C99 or later (no // comments, no C99 types like _Bool or
uint32_t, etc). I think the common pattern would be to use `unsigned`, which is
also variably-sized (as are all the integer types, technically, thanks to
CHAR_BIT), but we do have one existing use of `size_t` in the file, which is
probably the best type to use there given that we expect people to assign the
results of `sizeof` into it. WDYT about using `size_t`?
I don't have the historical context to know whether we expect this header to be
C89 compatible, so it's not clear to me how disruptive it would be to use
stdint.h. One alternative would be to use stdint.h if it's available (via
`__has_include`) and otherwise fallback on a C89 custom definition of uint32_t,
but that strikes me as being more likely to introduce problems on systems we
don't test on or for compilers we don't test with.
> 2. Should `int excludeDeclarationsFromPCH` and `int displayDiagnostics`
> currently passed to `clang_createIndex()` also be included in the struct?
> Then only a single argument will be passed to
> `clang_createIndexWithOptions()`: `CXIndexOptions`.
I think that makes sense to me. It does raise the question of whether we want
to pack these boolean-like fields together, as in:
struct CXIndexOptions {
size_t Size;
int ExcludeDeclsFromPCH : 1;
int DisplayDiagnostics : 1;
int Reserved : 30;
const char *PreambleStoragePath;
...
};
This makes it a little less likely to need to grow the structure when adding
new options.
> 3. `clang_createIndex()` initializes global options based on environment
> variable values:
>
> if (getenv("LIBCLANG_BGPRIO_INDEX"))
> CIdxr->setCXGlobalOptFlags(CIdxr->getCXGlobalOptFlags() |
>
> CXGlobalOpt_ThreadBackgroundPriorityForIndexing);
> if (getenv("LIBCLANG_BGPRIO_EDIT"))
> CIdxr->setCXGlobalOptFlags(CIdxr->getCXGlobalOptFlags() |
>
> CXGlobalOpt_ThreadBackgroundPriorityForEditing);
>
> The recommended in documentation usage of `clang_CXIndex_setGlobalOptions` is:
>
> * \code
> * CXIndex idx = ...;
> * clang_CXIndex_setGlobalOptions(idx,
> * clang_CXIndex_getGlobalOptions(idx) |
> * CXGlobalOpt_ThreadBackgroundPriorityForIndexing);
> * \endcode
>
> So making these options part of `struct CXIndexOptions` and deprecating
> `clang_CXIndex_setGlobalOptions` requires introducing another global function
> that would read the environment variables:
>
> CINDEX_LINKAGE unsigned clang_getDefaultGlobalOptions();
>
> Is this the right approach?
Hmm, to make this patch easier, I think we might want to leave the environment
variable behavior alone and not shift these into the options structure (yet?).
Naively, I think it makes sense for these to eventually live in the options
structure, but we could expose them in a few different ways (an option to
prefer the env variable over a manual value as it is today or an option to
prefer the manual value over the env variable for folks who want more hermetic
behavior). WDYT? My opinion here isn't super strong, so if you have a strong
desire to deprecate and add a replacement API, I think that's a defensible
course to take.
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
