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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
