collinbaker marked 5 inline comments as done.
collinbaker added a comment.
Changed as requested. Again leaving it up to a committer to commit this
================
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.
+ */
----------------
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
================
Comment at: clang/tools/libclang/CXType.cpp:13
+#include "CXType.h"
#include "CIndexer.h"
----------------
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.
================
Comment at: clang/tools/libclang/CXType.cpp:404
+ if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
return FD->getBitWidthValue(getCursorContext(C));
}
----------------
vedgy wrote:
> I thought of the same fix while analyzing the assertion failure here:
> https://bugs.kde.org/show_bug.cgi?id=438249#c19
>
> An alternative implementation is to place this same check in
> `clang::FieldDecl::getBitWidthValue()`. This looks like a general issue that
> could affect non-libclang users of `clang::FieldDecl::getBitWidthValue()`.
> But apparently no one else has stumbled upon it.
>
> `clang::FieldDecl::getBitWidthValue()` returns `unsigned` and has no
> error-reporting mechanism yet. It could return
> `std::numeric_limits<unsigned>::max()` (or `0` if that's an invalid bit width
> value) in case of the value dependence.
This would be suitable for a follow-up, since it doesn't affect the public
interface of libclang.
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
