[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:

>> 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.

On second thought, the proposed `clang_getDefaultGlobalOptions()` API already offers users a choice to either prefer or override each option value from the env. variables, just like the existing `clang_CXIndex_[gs]etGlobalOptions` API. The environment variables are binary options: an existence, not value, of an env. variable determines an initial option value. So I don't understand what are the two different ways to expose these options.

In D143418#4130042 <https://reviews.llvm.org/D143418#4130042>, @aaron.ballman wrote:

> In D143418#4126266 <https://reviews.llvm.org/D143418#4126266>, @vedgy wrote:
>
>> 
>
> I've been trying to think of benefits for using a fixed-size integer type and the closest I can come is the consistency of the structure size across targets, but I don't think we need that consistency. I don't have a strong preference for `unsigned` vs `size_t`, so how about we go with your slight preference for `unsigned` unless someone finds a reason to use something else?

Sounds good. Here is my current WIP API version:

  typedef struct CXIndexOptions {
    /**
     * The size of struct CIndexOptions used for option versioning.
     *
     * Always assign sizeof(CIndexOptions) to this member right after
     * creating a CXIndexOptions object.
     */
    unsigned Size;
    /**
     * \see clang_createIndex()
     */
    int ExcludeDeclarationsFromPCH : 1;
    int DisplayDiagnostics : 1;
    /**
     * The path to a directory, in which to store temporary PCH files. If null or
     * empty, the default system temporary directory is used. These PCH files are
     * deleted on clean exit but stay on disk if the program crashes or is killed.
     *
     * Libclang does not create the directory at the specified path in the file
     * system. Therefore it must exist, or storing PCH files will fail.
     */
    const char *PreambleStoragePath;
    /**
     * Specifies a path which will contain log files for certain libclang
     * invocations. A null value implies that libclang invocations are not logged.
     */
    const char *InvocationEmissionPath;
  } CXIndexOptions;
  
  /**
   * Provides a shared context for creating translation units.
   *
   * Call this function instead of clang_createIndex() if you need to configure
   * the additional options in CXIndexOptions.
   *
   * \returns The created index or null in case of error, such as an unsupported
   * value of options->Size.
   *
   * For example:
   * \code
   * CXIndex createIndex(const char *ApplicationTemporaryPath) {
   *   const int ExcludeDeclarationsFromPCH = 1;
   *   const int DisplayDiagnostics = 1;
   * #if CINDEX_VERSION_MINOR >= 64
   *   CIndexOptions Opts;
   *   Opts.Size = sizeof(CIndexOptions);
   *   Opts.ExcludeDeclarationsFromPCH = ExcludeDeclarationsFromPCH;
   *   Opts.DisplayDiagnostics = DisplayDiagnostics;
   *   Opts.PreambleStoragePath = ApplicationTemporaryPath;
   *   Opts.InvocationEmissionPath = NULL;
   *   CIndex Idx = clang_createIndexWithOptions(&Opts);
   *   if (Idx)
   *     return Idx;
   *   fprintf(stderr, "clang_createIndexWithOptions() failed. "
   *                   "CINDEX_VERSION_MINOR = %d, sizeof(CIndexOptions) = %u",
   *           CINDEX_VERSION_MINOR, Opts.Size);
   * #else
   *   (void)ApplicationTemporaryPath;
   * #endif
   *
   *   return clang_createIndex(ExcludeDeclarationsFromPCH, DisplayDiagnostics);
   * }
   * \endcode
   *
   * \sa clang_createIndex()
   */
  CINDEX_LINKAGE CXIndex
  clang_createIndexWithOptions(const CXIndexOptions *options);




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143418/new/

https://reviews.llvm.org/D143418