[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

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