[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Igor Kushnir via Phabricator via cfe-commits]

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