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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.


================
Comment at: clang/tools/c-index-test/c-index-test.c:79
+    Opts.PreambleStoragePath = NULL;
+    Opts.InvocationEmissionPath = getenv("CINDEXTEST_INVOCATION_EMISSION_PATH");
+
----------------
aaron.ballman wrote:
> 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?
The disadvantages of committing to defaulting to `0`:
1. The usage you propose is still more verbose and error-prone than
```
CXIndexOptions Opts = clang_getDefaultIndexOptions();
Opts.Whatever = 12;
CXIndex Idx = clang_createIndexWithOptions(&Opts);
```
2. The `memset` would look very unclean in modern C++ code.
3. The `0` commitment may force unnatural naming of a future option to invert its meaning.

The advantages:
1. No need to implement the inline function now.
2. Faster, but I am sure this performance difference doesn't matter. Even a non-inline function call itself (even if it did nothing) to `clang_createIndexWithOptions()` should take longer than assigning a few values to built-in-typed members.

Another advantage of not having to remember to update the inline function's implementation when new options are added is counterbalanced by the need to be careful to add new options in backwards compatible way where `0` is the default.

Any other advantages of the `0` that I miss? Maybe there are some advantages for C users, but I suspect most libclang users are C++.


================
Comment at: clang/tools/c-index-test/c-index-test.c:2079
+  if (!Idx)
+    return -1;
 
----------------
aaron.ballman wrote:
> 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.
I have noticed that these functions sometimes return `-1` and sometimes `1` on different errors. I thought that perhaps there is some difference in these two return values, but I couldn't figure out what it might be. Since telling the difference is not straightforward, you are probably right that there is //no// meaningful difference.


================
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,
----------------
aaron.ballman wrote:
> 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);
> ```
You are probably right looking forward.

I have opted for `!=` because this is the initial struct version. So right now a smaller struct size means something very unexpected or a failure to assign correct size on the user side. The advantage of `!=` is then more reliable catching of bugs in freshly written user code that uses the struct and an added flexibility of being able to //reduce// the struct's size in the next revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418