vedgy added a comment. Please update the commit message (there is no `clang_isFieldDeclBitWidthDependent` anymore) and update the revision with `arc diff --verbatim @~`.
================ Comment at: clang/include/clang-c/Index.h:3552 + * If the cursor does not reference a bit field declaration or if the bit + * field's width does not depend on template parameters, 0 is returned. + */ ---------------- collinbaker wrote: > vedgy wrote: > > vedgy wrote: > > > collinbaker wrote: > > > > vedgy wrote: > > > > > I just thought how the new API could be used in KDevelop. Currently > > > > > when `clang_getFieldDeclBitWidth()` is positive, e.g. 2, KDevelop > > > > > shows ` : 2` after the data member name in a tooltip. Ideally a > > > > > template-param-dependent expression (actual code) would be displayed > > > > > after the colon. If that's difficult to implement, `: > > > > > [tparam-dependent]` or `: ?` could be displayed instead. But it would > > > > > be more convenient and efficient to get this information by a single > > > > > call to `clang_getFieldDeclBitWidth()` instead of calling > > > > > `clang_isFieldDeclBitWidthDependent()` each time > > > > > `clang_getFieldDeclBitWidth()` returns `-1`. So how about returning > > > > > `-2` or `0` from `clang_getFieldDeclBitWidth()` instead of adding > > > > > this new API? > > > > I understand the motivation but I don't think requiring an extra call > > > > is asking too much of libclang clients, and it's one extra call that > > > > doesn't do much work and will be called rarely so I don't see > > > > efficiency concerns. Without strong reasons otherwise I think it's > > > > better to be explicit here. > > > KDevelop calls `clang_getFieldDeclBitWidth()` for each encountered class > > > member declaration. `clang_isFieldDeclBitWidthDependent()` would have to > > > be called each time `clang_getFieldDeclBitWidth()` returns `-1`, which > > > would be most of the time, because few class members are bit-fields. The > > > work this new function does is the same as that of > > > `clang_getFieldDeclBitWidth()` (repeated). > > > > > > If the concern about returning `-2` from `clang_getFieldDeclBitWidth()` > > > is cryptic return codes, an `enum` with named constants can be introduced. > > > > > > If the concern is breaking backward compatibility for users that relied > > > on the returned value being positive or `-1`, then a replacement for > > > `clang_getFieldDeclBitWidth()` with the most convenient API should be > > > introduced and `clang_getFieldDeclBitWidth()` itself - deprecated. > > > > > > KDevelop simply stores the value returned by > > > `clang_getFieldDeclBitWidth()` in an `int16_t m_bitWidth` data member and > > > uses it later. So if `-2` is returned, the only place in code to adjust > > > would be the use of this data member. With the current > > > `clang_isFieldDeclBitWidthDependent()` implementation, this function > > > would have to be called, `-2` explicitly stored in `m_bitWidth` and the > > > use of `m_bitWidth` would have to be adjusted just the same. > > > > > > Have you considered potential usage of the added API in your project? > > > Which alternative would be more convenient to use? > > One more API alternative is to replace > > `clang_isFieldDeclBitWidthDependent()` with `clang_isBitFieldDecl()`. The > > usage would be more straightforward and efficient: first call > > `clang_isBitFieldDecl()`; if it returns true (should be rare enough), call > > `clang_getFieldDeclBitWidth()`; if that returns `-1`, then the bit-field > > width must be unknown (dependent on template parameters). Such usage would > > still be less convenient and efficient compared to > > `clang_getFieldDeclBitWidth()` returning `-2` though. > Implemented as `clang_isBitFieldDecl` rather than magic return value Thanks. That's good enough for me. ================ Comment at: clang/tools/libclang/CXType.cpp:13 +#include "CXType.h" #include "CIndexer.h" ---------------- collinbaker wrote: > vedgy wrote: > > I guess //clang-format// did this include reordering. But it certainly > > looks out of place and the include order becomes wrong. So I think it > > should be reverted. > I don't agree, it's pretty standard for a source file to have its associated > header include at the top. You are right, I haven't realized the header-source association. The diff is still unrelated to the patch. But I'm no longer sure what's right, so won't insist on anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130303/new/ https://reviews.llvm.org/D130303 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
