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