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

[Aaron Ballman via Phabricator via cfe-commits]

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