[PATCH] D143418: [libclang] Add API to override preamble storage path

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang-c/Index.h:319
+   */
+  size_t Size;
+  /**
----------------
vedgy wrote:
> The type is `size_t` instead of the agreed upon `unsigned`, because the addition of `unsigned GlobalOptions` below means that `unsigned Size` no longer reduces the overall struct's size.
SGTM, thanks for the explanation


================
Comment at: clang/include/clang-c/Index.h:353
+ */
+CINDEX_LINKAGE unsigned clang_getDefaultGlobalOptions();
+
----------------
Don't want this to be a K&R C interface in pre-C2x modes.


================
Comment at: clang/tools/c-index-test/c-index-test.c:79
+    Opts.PreambleStoragePath = NULL;
+    Opts.InvocationEmissionPath = getenv("CINDEXTEST_INVOCATION_EMISSION_PATH");
+
----------------
vedgy wrote:
> When a libclang user needs to override a single option in `CXIndexOptions`, [s]he has to set every member of the struct explicitly. When new options are added, each libclang user needs to update the code that sets the options under `CINDEX_VERSION_MINOR` `#if`s. Accidentally omitting even one member assignment risks undefined or wrong behavior. How about adding an `inline` helper function `CXIndexOptions clang_getDefaultIndexOptions()`, which assigns default values to all struct members? Libclang users can then call this function to obtain the default configuration, then tweak only the members they want to override.
> 
> If this suggestion is to be implemented, how to deal with [[https://stackoverflow.com/questions/68004269/differences-of-the-inline-keyword-in-c-and-c|the differences of the inline keyword in C and C++]]?
By default, `0` should give you the most reasonable default behavior for most of the existing options (and new options should follow the same pattern). Ideally, users should be able to do:
```
CXIndexOptions Opts;
memset(&Opts, 0, sizeof(Opts));
Opts.Size = sizeof(Opts);
Opts.Whatever = 12;
CXIndex Idx = clang_createIndexWithOptions(&Opts);
```
Global options defaulting to 0 is fine (uses regular thread priorities), we don't think want to default to excluding declarations from PCH, and we want to use the default preamble and invocation emission paths (if any). The only option that nonzero as a default *might* make sense for is displaying diagnostics, but even that seems reasonable to expect the developer to manually enable.

So I don't know that we need a function to get us default indexing options as `0` should be a reasonable default for all of the options. As we add new options, we need to be careful to add them in backwards compatible ways where `0` means "do the most likely thing".

WDYT?


================
Comment at: clang/tools/c-index-test/c-index-test.c:2079
+  if (!Idx)
+    return -1;
 
----------------
vedgy wrote:
> Not sure `-1` is the right value to return here. This return value becomes the exit code of the `c-index-test` executable.
I think `-1` is fine -- the important thing is a nonzero return code so it's logged as an error rather than a valid result.


================
Comment at: clang/tools/libclang/CIndex.cpp:3691
 
+unsigned clang_getDefaultGlobalOptions() {
+  unsigned GlobalOptions = CXGlobalOpt_None;
----------------



================
Comment at: clang/tools/libclang/CIndex.cpp:3701-3702
+CXIndex clang_createIndexWithOptions(const CXIndexOptions *options) {
+  if (options->Size != sizeof(CXIndexOptions))
+    return NULL;
+  CIndexer *CIdxr = clang_createIndex_Impl(options->ExcludeDeclarationsFromPCH,
----------------
I think we want this to be `>` and not `!=`, maybe.

If the sizes are equal, we're on the happy path.

If the options from the caller are smaller than the options we know about, that should be okay because we won't attempt read the options not provided and instead rely on the default behavior being reasonable.

If the options from the caller are larger than the options we know about, we have to assume the user set an option we can't handle, and thus failing the request is reasonable.

So the way I'm envisioning us reading the options is:
```
if (options->Size >= offsetof(CXIndexOptions, FieldWeCareAbout))
  do_something(options->FieldWeCareAbout);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418