[ffigen] Cpp unique ptr ownership - #3513
Conversation
| if (method.isConstructor) { | ||
| returnTypeString = '$originalName*'; | ||
| params = method.parameters.map(paramDecl).join(', '); | ||
| final callArgs = method.parameters.map((p) => p.name).join(', '); |
There was a problem hiding this comment.
A constructor can take a std::unique_ptr too. Can you add a test for this case?
|
|
||
| final callArgs = method.parameters | ||
| .map((p) { | ||
| if (p.type is CppClassPointerType && |
There was a problem hiding this comment.
This is complicated enough that it should probably be a separate private util function, for readability.
|
|
||
| // Check if any method uses unique_ptr (owned ownership) so we can emit | ||
| // #include <memory> conditionally rather than always. | ||
| final needsMemoryHeader = methods.any( |
There was a problem hiding this comment.
There's no real harm in unconditionally including <memory>. I'd rather keep things simple.
| params = ['$selfType* self', ...otherParams].join(', '); | ||
| body = '${returnPrefix}self->${method.originalName}($callArgs);'; | ||
| final isUniquePtrReturn = | ||
| method.returnType is CppClassPointerType && |
There was a problem hiding this comment.
DRY. You're using this pattern of checking whether something is a unique_ptr often enough that it should be a util. Code duplication adds a maintenance burden over time.
| final m = method.originalName; | ||
| body = '${returnPrefix}self->$m($callArgs).release();'; | ||
| } else { | ||
| body = |
There was a problem hiding this comment.
DRY. These two cases are almost identical. A cleaner approach would be something like:
final methodName = method.originalName;
final suffix = isUniquePtrReturn ? 'release()' : '';
body = '${returnPrefix}self->$methodName($callArgs)$suffix;'
| final spelling = clang.clang_getTypeSpelling(cxtype).toStringAndDispose(); | ||
| if (spelling.contains('unique_ptr<')) { | ||
| final declCursor = clang.clang_getTypeDeclaration(cxtype); | ||
| final usr = clang.clang_getCursorUSR(declCursor).toStringAndDispose(); |
There was a problem hiding this comment.
We have extension methods for this. You can use declCursor.usr().
| if (spelling.contains('unique_ptr<')) { | ||
| final declCursor = clang.clang_getTypeDeclaration(cxtype); | ||
| final usr = clang.clang_getCursorUSR(declCursor).toStringAndDispose(); | ||
| final isStdUniquePtr = usr.contains('std@') && usr.contains('unique_ptr'); |
There was a problem hiding this comment.
Can you show me what one of these USRs looks like? Doing .contains here is a little odd. I would have thought you could just do final isStdUniquePtr = usr == "some string literal";. Is there a reason that won't work?
There was a problem hiding this comment.
libclang produces USRs in the following format:
c:@N@std@S@unique_ptr>#$@S@Node#$@N@std@S@default_delete>#S0_
So i think libclang appends the template argument types (such as >#$@s@Node...) directly into the USR string for each template instantiation, the full USR varies depending on the type parameter T.
There was a problem hiding this comment.
Ok. Can you do usr.startsWith("c:@N@std@S@unique_ptr>#")?
| final usr = clang.clang_getCursorUSR(declCursor).toStringAndDispose(); | ||
| final isStdUniquePtr = usr.contains('std@') && usr.contains('unique_ptr'); | ||
| if (isStdUniquePtr) { | ||
| final spelling = clang |
|
|
||
| final rawPtrVars = <String, String>{}; | ||
| for (final p in ownedParams) { | ||
| rawPtrVars[p.name] = '_raw_${p.name}'; |
There was a problem hiding this comment.
Creating these sorts of variables is exactly what LocalVariables is for. It would be better to use that. But actually, now that you've got the detachPointer method, you can probably inline it, and get rid of these local variables.
| } | ||
|
|
||
| /// A pointer to a C++ class wrapper object. | ||
| /// Returned pointers are always unowned by default. The developer must call |
There was a problem hiding this comment.
Restore this deleted comment?
| final ownershipChecks = StringBuffer(); | ||
| for (final p in ownedParams) { | ||
| final raw = rawPtrVars[p.name]!; | ||
| ownershipChecks.write(' final $raw = ${p.name}.detachPointer();\n'); |
There was a problem hiding this comment.
The detachPointer() call should happen in CppUniquePtrType.convertDartTypeToFfiDartType
| if (method.isStatic) { | ||
| final callLine = hasReturn | ||
| ? 'return $returnExpr;' | ||
| : '$glue($callArgs);'; |
There was a problem hiding this comment.
Can $glue($callArgs) be replaced with returnExpr?
| } else { | ||
| final callLine = hasReturn | ||
| ? 'return $returnExpr;' | ||
| : '$glue($callArgs);'; |
There was a problem hiding this comment.
Can $glue($callArgs) be replaced with returnExpr?
| } | ||
| '''); | ||
| } else { | ||
| final callLine = hasReturn |
There was a problem hiding this comment.
This callLine variable is identical between both branches of this if statement, so you could deduplicate it by just moving it up next to the definition of hasReturn.
| params = ['$selfType* self', ...otherParams].join(', '); | ||
| body = '${returnPrefix}self->${method.originalName}($callArgs);'; | ||
| final methodName = method.originalName; | ||
| final suffix = method.returnType is CppUniquePtrType |
There was a problem hiding this comment.
This is fine for now, but you're probably going to run into more places where you need to reuse this return type conversion (and the _cppCallArg util), eg when adding support for top-level functions with C++ signatures.
I filed a bug to clean this up later, don't worry about it at the moment: #3523
2 participants
No description provided.