[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Aaron Ballman via Phabricator via cfe-commits]

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