aaron.ballman 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");
+
----------------
vedgy wrote:
> 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++.
>The usage you propose is still more verbose and error-prone than
> ```
> CXIndexOptions Opts = clang_getDefaultIndexOptions();
> Opts.Whatever = 12;
> CXIndex Idx = clang_createIndexWithOptions(&Opts);
> ```
I see it as being roughly the same amount of verbosity and proneness to error,
but maybe that's just me.
> The memset would look very unclean in modern C++ code.
I don't see this as a problem. 1) `std::memset` gets plenty of usage in modern
C++ still, 2) you can also initialize with ` = { sizeof(CXIndexOptions) };` and
rely on the zero init for subsequent members after the first (personally, I
think that's too clever, but reasonable people may disagree), 3) this is a C
API, folks using C++ may wish to wrap it in a better interface for C++ anyway.
> The 0 commitment may force unnatural naming of a future option to invert its
> meaning.
I do worry about this one, especially given there's no way to predict what
future options we'll need. But it also forces us to think about what the
correct default behavior should be, whereas if we push it off to a helper
function, we make it harder for everyone who didn't know to use that helper
function for whatever reason.
> 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++.
Familiarity for those who are used to dealing with existing C APIs that behave
this way (like Win32 APIs), but that can probably be argued either way. My
guess is that all libclang users are having to work through the C interface,
and some decently large amount of them do so from languages other than C. But
whether that means most are C++ users or not is questionable -- I know I've
seen uses from Python and Java as well as C++.
I'm not strongly opposed to having an API to get the default values, but I
don't see a lot of value in it either. It's something we could always add later
if we find there's a need for it in practice.
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
