vedgy added a comment. In D143418#4125756 <https://reviews.llvm.org/D143418#4125756>, @aaron.ballman wrote:
> 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. `size_t` indeed makes logical sense for this member as that's the return type of `sizeof`. `size_t` is two times larger than `unsigned` on x86_64, but I don't think the size of this struct has any chance of impacting performance. Though it wouldn't hurt to pack the size and the boolean options in a single-pointer-sized region on x86_64. After all, this struct's size will never reach `UINT_MAX`. I slightly prefer `unsigned` due to my efficiency inclinations :). What do you prefer? Is there any benefit in using a fixed-size integer type here? >> 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. When we add new options, the struct's size must grow in order to distinguish different struct versions and prevent undefined behavior! If a member is added within the same LLVM release, it and other members added in that release can be reordered and packed to minimize the size. For example, I plan to add a `bool StorePreamblesInMemory` in LLVM 17. While adding it, I can reorder and pack anything I like as the whole struct is introduced in this version. But there is no need to reserve anything, I think. This is assuming we don't need to support compatibility at each intermediate commit of libclang. >> 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. OK, let's skip the global options member for now. I'll add a `\todo` about this. 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
