Hello,
I just implemented my first function in libclang – it gets a field cursor and returns the bitfield width, if that’s a bitfield. Otherwise it returns a negative value.
I’d like to submit it as a patch (never did that before, I’ll read up on how to do that later), but first can someone review this routine? It seems to work, but having never written anything inside clang (and having played with clang for only a couple of weeks now), I suspect there may just be half a dozen bugs there. In any case, I hope it’ll be helpful at least for some people reading this list.
And by the way, kudos to all Clang developers for the high quality product. The code (at least the bits I looked at when implementing this) is a delight to read.
// -1 : cursor is not a field declaration
// -2 : cursor is not a bitfield declaration
// -3 : could not evaluate the bitfield width
int clang_Cursor_getBitfieldWidth(CXCursor C) {
if (clang_isDeclaration(C.kind)) {
Decl *D = cxcursor::getCursorDecl(C);
if( const FieldDecl *FD = dyn_cast_or_null< FieldDecl >(D) ){
Expr *pBitWidth = FD->getBitWidth();
if( pBitWidth ){
llvm::APSInt width;
if( pBitWidth->EvaluateAsInt(width, getCursorASTUnit(C)->getASTContext() ) ){
return *width.getRawData();
}
return -3;
}
return -2;
}
}
return -1;
}
Sincerely,
Sergiy Migdalskiy
You should use FD->getBitWidthValue(ASTContext) to get the width. Before that call, you should also bail out with an error if pBitWidth->isValueDependent().
Hello,
I just implemented my first function in libclang – it gets a field
cursor and returns the bitfield width, if that’s a bitfield. Otherwise
it returns a negative value.
This is just what I need.
I’d like to submit it as a patch (never did that before, I’ll read up on
how to do that later), but first can someone review this routine? It
seems to work, but having never written anything inside clang (and
having played with clang for only a couple of weeks now), I suspect
there may just be half a dozen bugs there. In any case, I hope it’ll be
helpful at least for some people reading this list.
And by the way, kudos to all Clang developers for the high quality
product. The code (at least the bits I looked at when implementing this)
is a delight to read.
// -1 : cursor is not a field declaration
// -2 : cursor is not a bitfield declaration
// -3 : could not evaluate the bitfield width
int clang_Cursor_getBitfieldWidth(CXCursor C) {
if (clang_isDeclaration(C.kind)) {
Decl *D = cxcursor::getCursorDecl(C);
if( const FieldDecl *FD = dyn_cast_or_null< FieldDecl >(D) ){
Expr *pBitWidth = FD->getBitWidth();
if( pBitWidth ){
llvm::APSInt width;
if( pBitWidth->EvaluateAsInt(width,
getCursorASTUnit(C)->getASTContext() ) ){
return *width.getRawData();
}
return -3;
}
return -2;
}
}
return -1;
}
Sincerely,
Sergiy Migdalskiy
Do we really need three different values to indicate a failure? Either you can get the width of the bitfield or you can't.
Ok, does everyone agree on this variant?
Also, is the check "isValueDependent()" essentially for the cases when bitwidth is templatized? Or is there some other reason?
// Return bitfield bit width. Cursor must be a value-independent bit field declaration
int clang_Cursor_getBitfieldWidth(CXCursor C) {
if (clang_isDeclaration(C.kind)) {
Decl *D = cxcursor::getCursorDecl(C);
if( const FieldDecl *FD = dyn_cast_or_null< FieldDecl >(D) ){
Expr *pBitWidth = FD->getBitWidth();
if( pBitWidth && !pBitWidth->isValueDependent() )
return FD->getBitWidthValue( getCursorASTUnit(C)->getASTContext() );
}
}
return -1;
}
Sincerely,
Sergiy Migdalskiy
Ok, does everyone agree on this variant?
Also, is the check “isValueDependent()” essentially for the cases when bitwidth is templatized? Or is there some other reason?
Yes, exactly, it’s for the case where the bitwidth involves a template argument in a way which means we can’t evaluate it.
// Return bitfield bit width. Cursor must be a value-independent bit field declaration
int clang_Cursor_getBitfieldWidth(CXCursor C) {
if (clang_isDeclaration(C.kind)) {
Decl *D = cxcursor::getCursorDecl(C);
if( const FieldDecl *FD = dyn_cast_or_null< FieldDecl >(D) ){
Expr *pBitWidth = FD->getBitWidth();
if( pBitWidth && !pBitWidth->isValueDependent() )
return FD->getBitWidthValue( getCursorASTUnit(C)->getASTContext() );
}
}
return -1;
}
Yes, this looks fine to me, functionally. There are some coding style violations here (variable names, whitespace). See:
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
http://llvm.org/docs/CodingStandards.html#spaces-before-parentheses
The next thing to do is to get a complete patch together and mail it to cfe-commits@ for a proper review. This patch should include test cases (maybe extend c-index-test.c’s PrintCursor to print out the bit width for bitfields, and extend one of the test/Index/* tests to make sure the correct value is produced).
Thanks!
Richard