Run-time type information support #141
Conversation
Source/buildbindingccpp.go
Outdated
| w.Writeln(" static_assert(std::is_convertible<T, CBase>::value, \"T must be convertible to %s::CBase\");", NameSpace) | ||
| w.Writeln("") | ||
| w.Writeln(" if (pObj && pObj->m_pWrapper->ImplementsInterface(pObj.get(), T::getClassName())){") | ||
| w.Writeln(" return std::make_shared<T>(pObj->m_pWrapper, pObj->m_pHandle);") |
There was a problem hiding this comment.
Need to call pObj->wrapper()->AcquireInstance(pObj); to make sure reference count is correct.
Source/buildbindingpython.go
Outdated
| w.Writeln(" @classmethod") | ||
| w.Writeln(" def cast(cls, instance):") | ||
| w.Writeln(" if instance and instance._wrapper.ImplementsInterface(instance, cls.ClassName()):") | ||
| w.Writeln(" return cls(instance._handle, instance._wrapper)") |
There was a problem hiding this comment.
Need to call instance._wrapper.AcquireInstance(instance); to make sure reference count is correct.
|
Looks quite good.
|
I think that might not actually be necessary, it depends on the other binding languages but so far I noticed:
@martinweismann, @alexanderoster do you see any problem with using this strategy for all other binding languages? Other languages might use an underscore prefix (I think this is a common pattern in JS for example). Edit: I forgot to mention, if these names do need to be configurable there's an additional complication as these are methods on the Base class, while the current pattern is to add attributes to the Global tag. Also, this would mean the API developer has to add the method to the Base class definition, but the return type for |
|
|
||
| using namespace RTTI; | ||
| while (auto animal = iter->GetNextAnimal()) { | ||
| if (auto tiger = rtti_cast<CTiger>(animal)) { |
There was a problem hiding this comment.
@alexanderoster @martinweismann how do you feel about the name of this method (<namespace>_cast), I think it would be better to change this to something like obj_cast because it already resides in the namespace.
For example, using it on a component with namespace Widgets would currently look like this:
bool CheckWidget(Widgets::PBase pObj) {
auto foo = Widgets::widgets_cast<Widgets::PFoo>(pObj);
return foo ? foo->IsOk() : false;
}Depending on the namespace, the method name can become awkward, especially when the namespace isn't imported.
There was a problem hiding this comment.
Good point.
Considering that class names in the ACT-C++ bindings are usually referred to as Namespace::CClassName, i think using Namespace::obj_cast is more consistent than Namespace::Namespace_cast.
|
Chorus detected one or more security issues with this pull request. See the Checks tab for more details. As a reminder, please follow the secure code review process as part of the Secure Coding Non-Negotiable requirement. |
|
Hi @alexanderoster and @martinweismann
Looking into comments on this PR I see next pending tasks:
Just want to double check if anything missed before taking over this tasks. |
| class := classes[j] | ||
| hash := class.classHash() | ||
| w.BeginLine() | ||
| w.Printf(" static const RTTI_uint8 s_%sHash[] = {", class.ClassName) |
There was a problem hiding this comment.
hard coded namespace "RTTI" should be replaced with "componentdefinition.namespace"
Same in line 757 and potentially more.
|
Hi @anpilog , thanks for picking this up. I don't see any fundamental problem left. @fsahmad 's approach is good. I would love to merge the fork back into develop to make this part of the new release. The list of open points is accurate, except for two items:
|
Add basic RTTI support to ACT to make it possible to perform dynamic casts on all binding objects.
Implements issue #142
Examples/RTTIRTTI::rtti_cast<T>castclassmethod to theBaseclassbool ImplementsInterface(CBase*, const std::string&)was introduced on theCWrapperclass, that usesdynamic_castto check if the object can be cast to the interfacePending tasks