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

[Collin Baker via Phabricator via cfe-commits]

collinbaker updated this revision to Diff 503952.
collinbaker added a comment.

Missed commit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/tools/libclang/CXType.cpp
  clang/tools/libclang/libclang.map


Index: clang/tools/libclang/libclang.map
===
--- clang/tools/libclang/libclang.map
+++ clang/tools/libclang/libclang.map
@@ -421,6 +421,7 @@
 LLVM_17 {
   global:
 clang_CXXMethod_isExplicit;
+clang_isBitFieldDecl;
 };
 
 # Example of how to add a new symbol version entry.  If you do add a new symbol
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,11 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,14 +371,27 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isBitFieldDecl(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const auto *FD = dyn_cast_or_null(D))
+  return FD->isBitField();
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
   if (clang_isDeclaration(C.kind)) {
 const Decl *D = getCursorDecl(C);
 
-if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+if (const auto *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,18 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a field declaration has a bit width expression.
+ *
+ * If the cursor does not reference a bit field declaration 0 is returned.
+ */
+CINDEX_LINKAGE unsigned clang_isBitFieldDecl(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is 
returned.
+ * If the cursor does not reference a bit field, or if the bit field's width
+ * expression cannot be evaluated, -1 is returned.
  */
 CINDEX_LINKAGE int clang_getFieldDeclBitWidth(CXCursor C);
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -290,6 +290,11 @@
 - Introduced the new function ``clang_CXXMethod_isExplicit``,
   which identifies whether a constructor or conversion function cursor
   was marked with the explicit identifier.
+- Added check in ``clang_getFieldDeclBitWidth`` for whether a bit field
+  has an evaluable bit width. Fixes undefined behavior when called on a
+  bit field whose width depends on a template paramter.
+- Added function ``clang_isBitFieldDecl`` to check if a struct/class field is a
+  bit field.
 
 Static Analyzer
 ---


Index: clang/tools/libclang/libclang.map
===
--- clang/tools/libclang/libclang.map
+++ clang/tools/libclang/libclang.map
@@ -421,6 +421,7 @@
 LLVM_17 {
   global:
 clang_CXXMethod_isExplicit;
+clang_isBitFieldDecl;
 };
 
 # Example of how to add a new symbol version entry.  If you do add a new symbol
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,11 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,14 +371,27 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isBitFieldDecl(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const auto *FD = dyn_cast_or_null(D))
+  return FD->isBitField();
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
   if (clang_isDeclaration(C.kind)) {
 const Decl *D = getCursorDecl(C);
 
-if (const FieldDecl *FD = 

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker updated this revision to Diff 503950.
collinbaker marked 3 inline comments as done.
collinbaker edited the summary of this revision.
collinbaker added a comment.

Cleanups and split header order change to separate commit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/tools/libclang/CXType.cpp
  clang/tools/libclang/libclang.map


Index: clang/tools/libclang/libclang.map
===
--- clang/tools/libclang/libclang.map
+++ clang/tools/libclang/libclang.map
@@ -421,6 +421,7 @@
 LLVM_17 {
   global:
 clang_CXXMethod_isExplicit;
+clang_isBitFieldDecl;
 };
 
 # Example of how to add a new symbol version entry.  If you do add a new symbol
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -371,14 +371,27 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isBitFieldDecl(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const auto *FD = dyn_cast_or_null(D))
+  return FD->isBitField();
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
   if (clang_isDeclaration(C.kind)) {
 const Decl *D = getCursorDecl(C);
 
-if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+if (const auto *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,18 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a field declaration has a bit width expression.
+ *
+ * If the cursor does not reference a bit field declaration 0 is returned.
+ */
+CINDEX_LINKAGE unsigned clang_isBitFieldDecl(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is 
returned.
+ * If the cursor does not reference a bit field, or if the bit field's width
+ * expression cannot be evaluated, -1 is returned.
  */
 CINDEX_LINKAGE int clang_getFieldDeclBitWidth(CXCursor C);
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -290,6 +290,11 @@
 - Introduced the new function ``clang_CXXMethod_isExplicit``,
   which identifies whether a constructor or conversion function cursor
   was marked with the explicit identifier.
+- Added check in ``clang_getFieldDeclBitWidth`` for whether a bit field
+  has an evaluable bit width. Fixes undefined behavior when called on a
+  bit field whose width depends on a template paramter.
+- Added function ``clang_isBitFieldDecl`` to check if a struct/class field is a
+  bit field.
 
 Static Analyzer
 ---


Index: clang/tools/libclang/libclang.map
===
--- clang/tools/libclang/libclang.map
+++ clang/tools/libclang/libclang.map
@@ -421,6 +421,7 @@
 LLVM_17 {
   global:
 clang_CXXMethod_isExplicit;
+clang_isBitFieldDecl;
 };
 
 # Example of how to add a new symbol version entry.  If you do add a new symbol
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -371,14 +371,27 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isBitFieldDecl(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const auto *FD = dyn_cast_or_null(D))
+  return FD->isBitField();
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
   if (clang_isDeclaration(C.kind)) {
 const Decl *D = getCursorDecl(C);
 
-if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+if (const auto *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,18 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a field 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

Found some minor cleanups but otherwise LGTM (feel free to fix when landing if 
you'd like).




Comment at: clang/tools/libclang/CXType.cpp:380-384
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
   if (FD->isBitField())
+return 1;
+}
+  }

The conversion is always going to do the right thing: 
https://eel.is/c++draft/conv#integral-2.sentence-2



Comment at: clang/tools/libclang/CXType.cpp:395
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())





Comment at: clang/tools/libclang/CXType.cpp:13
 
+#include "CXType.h"
 #include "CIndexer.h"

dexonsmith wrote:
> vedgy wrote:
> > 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.
> This is correct behaviour from clang-format.
> 
> Given that there were no functional changes to includes, typically we’d omit 
> clang-format cleanups. (I know there was a change below originally, but that 
> was reverted.)
> 
> I’m fine leaving the header clean-up in, or separating it out to different 
> NFC commit (ideal; could be done when pushing), or skipping it entirely. 
+1 -- FWIW, we usually ask for formatting changes to be separated out into a 
separate commit because it makes git blame more useful when we're doing code 
archeology later, but this is minor enough to be unlikely to cause an issue, so 
I don't feel strongly.


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

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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/tools/libclang/CXType.cpp:13
 
+#include "CXType.h"
 #include "CIndexer.h"

vedgy wrote:
> 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.
This is correct behaviour from clang-format.

Given that there were no functional changes to includes, typically we’d omit 
clang-format cleanups. (I know there was a change below originally, but that 
was reverted.)

I’m fine leaving the header clean-up in, or separating it out to different NFC 
commit (ideal; could be done when pushing), or skipping it entirely. 


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

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Collin Baker via Phabricator via cfe-commits]

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::max()` (or `0` if that's an invalid bit width 
> value) in case of the value dependence.
This would 

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker updated this revision to Diff 503570.
collinbaker edited the summary of this revision.
collinbaker added a comment.

Requested changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/tools/libclang/CXType.cpp
  clang/tools/libclang/libclang.map


Index: clang/tools/libclang/libclang.map
===
--- clang/tools/libclang/libclang.map
+++ clang/tools/libclang/libclang.map
@@ -421,6 +421,7 @@
 LLVM_17 {
   global:
 clang_CXXMethod_isExplicit;
+clang_isBitFieldDecl;
 };
 
 # Example of how to add a new symbol version entry.  If you do add a new symbol
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,11 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,7 +371,7 @@
   return ULLONG_MAX;
 }
 
-int clang_getFieldDeclBitWidth(CXCursor C) {
+unsigned clang_isBitFieldDecl(CXCursor C) {
   using namespace cxcursor;
 
   if (clang_isDeclaration(C.kind)) {
@@ -379,6 +379,21 @@
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
   if (FD->isBitField())
+return 1;
+}
+  }
+
+  return 0;
+}
+
+int clang_getFieldDeclBitWidth(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,18 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a field declaration has a bit width expression.
+ *
+ * If the cursor does not reference a bit field declaration 0 is returned.
+ */
+CINDEX_LINKAGE unsigned clang_isBitFieldDecl(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is 
returned.
+ * If the cursor does not reference a bit field, or if the bit field's width
+ * expression cannot be evaluated, -1 is returned.
  */
 CINDEX_LINKAGE int clang_getFieldDeclBitWidth(CXCursor C);
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -290,6 +290,11 @@
 - Introduced the new function ``clang_CXXMethod_isExplicit``,
   which identifies whether a constructor or conversion function cursor
   was marked with the explicit identifier.
+- Added check in ``clang_getFieldDeclBitWidth`` for whether a bit field
+  has an evaluable bit width. Fixes undefined behavior when called on a
+  bit field whose width depends on a template paramter.
+- Added function ``clang_isBitFieldDecl`` to check if a struct/class field is a
+  bit field.
 
 Static Analyzer
 ---


Index: clang/tools/libclang/libclang.map
===
--- clang/tools/libclang/libclang.map
+++ clang/tools/libclang/libclang.map
@@ -421,6 +421,7 @@
 LLVM_17 {
   global:
 clang_CXXMethod_isExplicit;
+clang_isBitFieldDecl;
 };
 
 # Example of how to add a new symbol version entry.  If you do add a new symbol
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,11 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,7 +371,7 @@
   return ULLONG_MAX;
 }
 
-int clang_getFieldDeclBitWidth(CXCursor C) {
+unsigned clang_isBitFieldDecl(CXCursor C) {
   using namespace cxcursor;
 
   if (clang_isDeclaration(C.kind)) {
@@ -379,6 +379,21 @@
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
   if (FD->isBitField())
+return 1;
+}
+  }
+
+  return 0;
+}
+
+int clang_getFieldDeclBitWidth(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D130303#4176330 , @dexonsmith 
wrote:

> In D130303#4175664 , @collinbaker 
> wrote:
>
>> @dexonsmith can you weigh in?
>
> Introducing `clang_isBitFieldDecl` sounds like a clean/straightforward 
> solution to me.

+1, that seems less mysterious than returning `-2` from 
`clang_getFieldDeclBitWidth()` to me.


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

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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D130303#4175664 , @collinbaker 
wrote:

> @dexonsmith can you weigh in?

Introducing `clang_isBitFieldDecl` sounds like a clean/straightforward solution 
to me.


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

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker added a comment.

@dexonsmith can you weigh in?


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

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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



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:
> 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.


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

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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



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:
> > 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?



Comment at: clang/tools/libclang/CXType.cpp:13
 
+#include "CXType.h"
 #include "CIndexer.h"

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.


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

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker marked 3 inline comments as done.
collinbaker added a comment.

Thanks for the review. Someone else will need to commit since I don't have 
permission.


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

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker updated this revision to Diff 502763.
collinbaker added a comment.

Add symbol to version map


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/tools/libclang/CXType.cpp
  clang/tools/libclang/libclang.map

Index: clang/tools/libclang/libclang.map
===
--- clang/tools/libclang/libclang.map
+++ clang/tools/libclang/libclang.map
@@ -421,6 +421,7 @@
 LLVM_17 {
   global:
 clang_CXXMethod_isExplicit;
+clang_isFieldDeclBitWidthDependent;
 };
 
 # Example of how to add a new symbol version entry.  If you do add a new symbol
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,11 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,6 +371,21 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isFieldDeclBitWidthDependent(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && FD->getBitWidth()->isValueDependent())
+return 1;
+}
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
@@ -378,7 +393,7 @@
 const Decl *D = getCursorDecl(C);
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,19 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a bit field's width depends on template parameters.
+ *
+ * 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.
+ */
+CINDEX_LINKAGE unsigned clang_isFieldDeclBitWidthDependent(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is returned.
+ * If a cursor that is not a bit field declaration is passed in, or if the bit
+ * field's width expression cannot be evaluated, -1 is returned.
  */
 CINDEX_LINKAGE int clang_getFieldDeclBitWidth(CXCursor C);
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -290,6 +290,11 @@
 - Introduced the new function ``clang_CXXMethod_isExplicit``,
   which identifies whether a constructor or conversion function cursor
   was marked with the explicit identifier.
+- Added check in ``clang_getFieldDeclBitWidth`` for whether a bit field
+  has an evaluable bit width. Fixes undefined behavior when called on a
+  bit field whose width depends on a template paramter.
+- Added function ``clang_isFieldDeclBitWidthDependent`` to check if a
+  bit field's width depends on a template parameter.
 
 Static Analyzer
 ---
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker added inline comments.



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:
> 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.


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

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker updated this revision to Diff 502762.
collinbaker added a comment.

Add release notes and remove unneeded include


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/tools/libclang/CXType.cpp


Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,11 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,6 +371,21 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isFieldDeclBitWidthDependent(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && FD->getBitWidth()->isValueDependent())
+return 1;
+}
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
@@ -378,7 +393,7 @@
 const Decl *D = getCursorDecl(C);
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,19 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a bit field's width depends on template parameters.
+ *
+ * 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.
+ */
+CINDEX_LINKAGE unsigned clang_isFieldDeclBitWidthDependent(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is 
returned.
+ * If a cursor that is not a bit field declaration is passed in, or if the bit
+ * field's width expression cannot be evaluated, -1 is returned.
  */
 CINDEX_LINKAGE int clang_getFieldDeclBitWidth(CXCursor C);
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -290,6 +290,11 @@
 - Introduced the new function ``clang_CXXMethod_isExplicit``,
   which identifies whether a constructor or conversion function cursor
   was marked with the explicit identifier.
+- Added check in ``clang_getFieldDeclBitWidth`` for whether a bit field
+  has an evaluable bit width. Fixes undefined behavior when called on a
+  bit field whose width depends on a template paramter.
+- Added function ``clang_isFieldDeclBitWidthDependent`` to check if a
+  bit field's width depends on a template parameter.
 
 Static Analyzer
 ---


Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,11 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,6 +371,21 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isFieldDeclBitWidthDependent(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && FD->getBitWidth()->isValueDependent())
+return 1;
+}
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
@@ -378,7 +393,7 @@
 const Decl *D = getCursorDecl(C);
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,19 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a bit field's width depends on 

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker updated this revision to Diff 502758.
collinbaker added a comment.

Sync with upstream changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130303/new/

https://reviews.llvm.org/D130303

Files:
  clang/include/clang-c/Index.h
  clang/tools/libclang/CXType.cpp


Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,12 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
+#include "clang-c/Index.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,6 +372,21 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isFieldDeclBitWidthDependent(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && FD->getBitWidth()->isValueDependent())
+return 1;
+}
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
@@ -378,7 +394,7 @@
 const Decl *D = getCursorDecl(C);
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,19 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a bit field's width depends on template parameters.
+ *
+ * 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.
+ */
+CINDEX_LINKAGE unsigned clang_isFieldDeclBitWidthDependent(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is 
returned.
+ * If a cursor that is not a bit field declaration is passed in, or if the bit
+ * field's width expression cannot be evaluated, -1 is returned.
  */
 CINDEX_LINKAGE int clang_getFieldDeclBitWidth(CXCursor C);
 


Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -10,11 +10,12 @@
 //
 //======//
 
+#include "CXType.h"
 #include "CIndexer.h"
 #include "CXCursor.h"
 #include "CXString.h"
 #include "CXTranslationUnit.h"
-#include "CXType.h"
+#include "clang-c/Index.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -371,6 +372,21 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isFieldDeclBitWidthDependent(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && FD->getBitWidth()->isValueDependent())
+return 1;
+}
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
@@ -378,7 +394,7 @@
 const Decl *D = getCursorDecl(C);
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -2887,10 +2887,19 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a bit field's width depends on template parameters.
+ *
+ * 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.
+ */
+CINDEX_LINKAGE unsigned clang_isFieldDeclBitWidthDependent(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is returned.
+ * If a cursor that is not a bit field declaration is passed in, or if the bit
+ * field's width expression cannot be evaluated, -1 is returned.
  */
 CINDEX_LINKAGE int clang_getFieldDeclBitWidth(CXCursor C);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

In D130303#3724392 , @dexonsmith 
wrote:

> In D130303#3724247 , @rnk wrote:
>
>> Pinging alternative reviewer +@dexonsmith for a libclang API addition
>
> Looks reasonable to me -- this only changes behaviour of the existing API 
> when there was corruption before -- but if the goal is to get a vendor of 
> libclang-as-a-stable-API to sign off, I can't help.
>
> @arphaman, if you're busy, is there someone else that could take a quick look?

This still LGTM, for the same reasons. Let's just ship this.


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

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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



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.
+ */

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?


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

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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:3560
+ * If a cursor that is not a bit field declaration is passed in, or if the bit
+ * field's width expression cannot be evaluated, -1 is returned.
  */

Append the following line to the commit message to properly reference and 
(hopefully) close the bug automatically:
```
Fixes: https://github.com/llvm/llvm-project/issues/56644
```



Comment at: clang/tools/libclang/CXType.cpp:18
 #include "CXType.h"
+#include "clang-c/Index.h"
 #include "clang/AST/Decl.h"

Since the code worked fine before, I don't think this include should be added.


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

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

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added subscribers: aaron.ballman, vedgy.
vedgy added a comment.

I just verified that this patch fixes a KDevelop crash 
 reported by several users. Thank 
you!

@aaron.ballman, could you please review this fix?




Comment at: clang/include/clang-c/Index.h:3554
+ */
+CINDEX_LINKAGE unsigned clang_isFieldDeclBitWidthDependent(CXCursor C);
+

New API has to be mentioned in the //libclang// section of 
//clang/docs/ReleaseNotes.rst// and in the //LLVM_17// block of 
//clang/tools/libclang/libclang.map//.



Comment at: clang/tools/libclang/CXType.cpp:404
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }

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::max()` (or `0` if that's an invalid bit width 
value) in case of the value dependence.


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

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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D130303#3724247 , @rnk wrote:

> Pinging alternative reviewer +@dexonsmith for a libclang API addition

Looks reasonable to me -- this only changes behaviour of the existing API when 
there was corruption before -- but if the goal is to get a vendor of 
libclang-as-a-stable-API to sign off, I can't help.

@arphaman, if you're busy, is there someone else that could take a quick look?


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a subscriber: dexonsmith.
rnk added a comment.

Pinging alternative reviewer +@dexonsmith for a libclang API addition


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

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker added a comment.

Pinging this


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Dmitri, do you know a good libclang point of contact for the new API?


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

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

[Collin Baker via Phabricator via cfe-commits]

collinbaker created this revision.
collinbaker added a reviewer: rnk.
Herald added a subscriber: arphaman.
Herald added a project: All.
collinbaker requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In a class template, a bit field's width may depend on a template
parameter. In this case the width expression cannot be evaluated.

Previously clang_getFieldDeclBitWidth() would assert, or cause memory
unsafety and return an invalid result if assertions are disabled.

This adds a check for this case which returns an error code. An
additional function clang_isFieldDeclBitWidthDependent() as added for
users to check for this case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130303

Files:
  clang/include/clang-c/Index.h
  clang/tools/libclang/CXType.cpp


Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -15,6 +15,7 @@
 #include "CXString.h"
 #include "CXTranslationUnit.h"
 #include "CXType.h"
+#include "clang-c/Index.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -377,6 +378,21 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isFieldDeclBitWidthDependent(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && FD->getBitWidth()->isValueDependent())
+return 1;
+}
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
@@ -384,7 +400,7 @@
 const Decl *D = getCursorDecl(C);
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -3545,10 +3545,19 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a bit field's width depends on template parameters.
+ *
+ * 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.
+ */
+CINDEX_LINKAGE unsigned clang_isFieldDeclBitWidthDependent(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is 
returned.
+ * If a cursor that is not a bit field declaration is passed in, or if the bit
+ * field's width expression cannot be evaluated, -1 is returned.
  */
 CINDEX_LINKAGE int clang_getFieldDeclBitWidth(CXCursor C);
 


Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -15,6 +15,7 @@
 #include "CXString.h"
 #include "CXTranslationUnit.h"
 #include "CXType.h"
+#include "clang-c/Index.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
@@ -377,6 +378,21 @@
   return ULLONG_MAX;
 }
 
+unsigned clang_isFieldDeclBitWidthDependent(CXCursor C) {
+  using namespace cxcursor;
+
+  if (clang_isDeclaration(C.kind)) {
+const Decl *D = getCursorDecl(C);
+
+if (const FieldDecl *FD = dyn_cast_or_null(D)) {
+  if (FD->isBitField() && FD->getBitWidth()->isValueDependent())
+return 1;
+}
+  }
+
+  return 0;
+}
+
 int clang_getFieldDeclBitWidth(CXCursor C) {
   using namespace cxcursor;
 
@@ -384,7 +400,7 @@
 const Decl *D = getCursorDecl(C);
 
 if (const FieldDecl *FD = dyn_cast_or_null(D)) {
-  if (FD->isBitField())
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }
   }
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -3545,10 +3545,19 @@
 CINDEX_LINKAGE unsigned long long
 clang_getEnumConstantDeclUnsignedValue(CXCursor C);
 
+/**
+ * Returns non-zero if a bit field's width depends on template parameters.
+ *
+ * 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.
+ */
+CINDEX_LINKAGE unsigned clang_isFieldDeclBitWidthDependent(CXCursor C);
+
 /**
  * Retrieve the bit width of a bit field declaration as an integer.
  *
- * If a cursor that is not a bit field declaration is passed in, -1 is returned.
+ * If a cursor that is not a bit field declaration is passed in, or if the bit
+ * field's width expression