aaron.ballman added a comment.
In D143418#4126266 <https://reviews.llvm.org/D143418#4126266>, @vedgy wrote:
> `size_t` indeed makes logical sense for this member as that's the return type
> of `sizeof`. `size_t` is two times larger than `unsigned` on x86_64, but I
> don't think the size of this struct has any chance of impacting performance.
Agreed.
> Though it wouldn't hurt to pack the size and the boolean options in a
> single-pointer-sized region on x86_64. After all, this struct's size will
> never reach `UINT_MAX`. I slightly prefer `unsigned` due to my efficiency
> inclinations :). What do you prefer? Is there any benefit in using a
> fixed-size integer type here?
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?
>>> 2. Should `int excludeDeclarationsFromPCH` and `int displayDiagnostics`
>>> currently passed to `clang_createIndex()` also be included in the struct?
>>> Then only a single argument will be passed to
>>> `clang_createIndexWithOptions()`: `CXIndexOptions`.
>>
>> I think that makes sense to me. It does raise the question of whether we
>> want to pack these boolean-like fields together, as in:
>>
>> struct CXIndexOptions {
>> size_t Size;
>>
>> int ExcludeDeclsFromPCH : 1;
>> int DisplayDiagnostics : 1;
>> int Reserved : 30;
>>
>> const char *PreambleStoragePath;
>> ...
>> };
>>
>> This makes it a little less likely to need to grow the structure when adding
>> new options.
>
> When we add new options, the struct's size must grow in order to distinguish
> different struct versions and prevent undefined behavior!
Oh gosh you're absolutely right, that was a think-o on my part.
> OK, let's skip the global options member for now. I'll add a `\todo` about
> this.
SGTM, thank you!
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
