implementing clone for ASTNodes

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

implementing clone for ASTNodes

Roman Popov via cfe-dev
I was wondering if anybody spend some efforts in implementing a clone Method for all ASTNodes?

It seems to me, that the current approach implemented in the ASTImporter is error-prone and hard to maintain.

Currently it looks to me that Decls / Stmt's are being probed for their type and then downcasted, eg.:

/* ASTNodeImporter::VisitExplicitCastExpr */
/* ASTImporter.cpp:6060 */
switch (E->getStmtClass()) {
  case Stmt::CStyleCastExprClass: {
     CStyleCastExpr *CCE = cast<CStyleCastExpr>(E);
     return CStyleCastExpr::Create(Importer.getToContext(), T,
        E->getValueKind(), E->getCastKind(),
        SubExpr, &BasePath, TInfo,
        Importer.Import(CCE->getLParenLoc()),
        Importer.Import(CCE->getRParenLoc()));
}
/* more cases*/


As the AST contains circular references and many nodes require an ASTContext
for construction, i'm wondering how feasible it would be to implement the following interface for each Decl?

 virtual Decl* clone(
   ASTContext* newContext, 
   DenseMap<Decl*, Decl*> ImportedDeclsCache = {}
 );






Sent with Mailtrack

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: implementing clone for ASTNodes

Roman Popov via cfe-dev
Hello Gaetano,

ASTImporter not just "imports" AST nodes. It also searches if they already exist in the destination AST. It is required to get a consistent AST.

I was thinking about "clone" approach some time ago, but the question how to handle complex dependencies is still open here.



18.01.2017 19:55, Gaetano Checinski via cfe-dev пишет:
I was wondering if anybody spend some efforts in implementing a clone Method for all ASTNodes?

It seems to me, that the current approach implemented in the ASTImporter is error-prone and hard to maintain.

Currently it looks to me that Decls / Stmt's are being probed for their type and then downcasted, eg.:

/* ASTNodeImporter::VisitExplicitCastExpr */
/* ASTImporter.cpp:6060 */
switch (E->getStmtClass()) {
  case Stmt::CStyleCastExprClass: {
     CStyleCastExpr *CCE = cast<CStyleCastExpr>(E);
     return CStyleCastExpr::Create(Importer.getToContext(), T,
        E->getValueKind(), E->getCastKind(),
        SubExpr, &BasePath, TInfo,
        Importer.Import(CCE->getLParenLoc()),
        Importer.Import(CCE->getRParenLoc()));
}
/* more cases*/


As the AST contains circular references and many nodes require an ASTContext
for construction, i'm wondering how feasible it would be to implement the following interface for each Decl?

 virtual Decl* clone(
   ASTContext* newContext, 
   DenseMap<Decl*, Decl*> ImportedDeclsCache = {}
 );






Sent with Mailtrack


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: implementing clone for ASTNodes

Roman Popov via cfe-dev
CC'ing cfe-dev again.

There should be no infinite loops in ASTImporter as they are broken by GetAlreadyImportedOrNull() method.
It is also interesting for me what will be the difference between clone() methods and methods of ASTImporter. Do you have any example?


18.01.2017 20:24, Gaetano Checinski пишет:
The ASTImporter creates a lookup table ( DenseMap<Decl*,Decl*> importedDecls), to keep track of the imported Decls and not to end up in a infinite loop.
This is why my proposed signature for the clone contains not only an ASTContext to clone into but also a `DenseMap<Decl*, Decl*>& ImportedDeclsCache` to keep track of those already imported Decls.


2017-01-18 17:16 GMT+00:00 Aleksei Sidorin <[hidden email]>:
Hello Gaetano,

ASTImporter not just "imports" AST nodes. It also searches if they already exist in the destination AST. It is required to get a consistent AST.

I was thinking about "clone" approach some time ago, but the question how to handle complex dependencies is still open here.



18.01.2017 19:55, Gaetano Checinski via cfe-dev пишет:
I was wondering if anybody spend some efforts in implementing a clone Method for all ASTNodes?

It seems to me, that the current approach implemented in the ASTImporter is error-prone and hard to maintain.

Currently it looks to me that Decls / Stmt's are being probed for their type and then downcasted, eg.:

/* ASTNodeImporter::VisitExplicitCastExpr */
/* ASTImporter.cpp:6060 */
switch (E->getStmtClass()) {
  case Stmt::CStyleCastExprClass: {
     CStyleCastExpr *CCE = cast<CStyleCastExpr>(E);
     return CStyleCastExpr::Create(Importer.getToContext(), T,
        E->getValueKind(), E->getCastKind(),
        SubExpr, &BasePath, TInfo,
        Importer.Import(CCE->getLParenLoc()),
        Importer.Import(CCE->getRParenLoc()));
}
/* more cases*/


As the AST contains circular references and many nodes require an ASTContext
for construction, i'm wondering how feasible it would be to implement the following interface for each Decl?

 virtual Decl* clone(
   ASTContext* newContext, 
   DenseMap<Decl*, Decl*> ImportedDeclsCache = {}
 );






Sent with Mailtrack
<img moz-do-not-send="true" class="m_5220777893620003423mailtrack-img" src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" height="0" width="0">


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: implementing clone for ASTNodes

Roman Popov via cfe-dev
The ASTImporter creates a lookup table ( DenseMap<Decl*,Decl*> importedDecls), to keep track of the imported Decls and not to end up in a infinite loop

>> There should be no infinite loops in ASTImporter as they are broken by GetAlreadyImportedOrNull() method.

I'm just justifying my proposal for the clone method. However there are more subtle ways how we can end up in an infinite loop/recursion.
I also appreciate your recent patch fixing one of those issues.

It is also interesting for me what will be the difference between clone() methods and methods of ASTImporter. Do you have any example?
My main concerns are maintainability, correctness and encapsulation.

Decl's are arranged in a class-hierarchy; they have additional data and are primary used through the Decl interface.
If we want to clone/import a concrete Decl we need:
- to query for the underlying type 
- downcast
- clone its containing data.

This approach ties the ASTImporter to all Decls and the ASTImporter must be kept in sync with the Decl's definitions, to maintain correctness. Additionally we have to break encapsulation by adding friend declarations in various Decl's to make the data accessible to the AST(Node)Importer.

Currently cloning/importing seems to be a second thought (if at all) and adds complexity due to the fact of being not implemented as a method of the Decl's. 


Sent with Mailtrack

2017-01-18 17:32 GMT+00:00 Aleksei Sidorin via cfe-dev <[hidden email]>:
CC'ing cfe-dev again.

There should be no infinite loops in ASTImporter as they are broken by GetAlreadyImportedOrNull() method.
It is also interesting for me what will be the difference between clone() methods and methods of ASTImporter. Do you have any example?


18.01.2017 20:24, Gaetano Checinski пишет:
The ASTImporter creates a lookup table ( DenseMap<Decl*,Decl*> importedDecls), to keep track of the imported Decls and not to end up in a infinite loop.
This is why my proposed signature for the clone contains not only an ASTContext to clone into but also a `DenseMap<Decl*, Decl*>& ImportedDeclsCache` to keep track of those already imported Decls.

<img class="m_6534975331693431624mailtrack-img" src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" height="0" width="0">

2017-01-18 17:16 GMT+00:00 Aleksei Sidorin <[hidden email]>:
Hello Gaetano,

ASTImporter not just "imports" AST nodes. It also searches if they already exist in the destination AST. It is required to get a consistent AST.

I was thinking about "clone" approach some time ago, but the question how to handle complex dependencies is still open here.



18.01.2017 19:55, Gaetano Checinski via cfe-dev пишет:
I was wondering if anybody spend some efforts in implementing a clone Method for all ASTNodes?

It seems to me, that the current approach implemented in the ASTImporter is error-prone and hard to maintain.

Currently it looks to me that Decls / Stmt's are being probed for their type and then downcasted, eg.:

/* ASTNodeImporter::VisitExplicitCastExpr */
/* ASTImporter.cpp:6060 */
switch (E->getStmtClass()) {
  case Stmt::CStyleCastExprClass: {
     CStyleCastExpr *CCE = cast<CStyleCastExpr>(E);
     return CStyleCastExpr::Create(Importer.getToContext(), T,
        E->getValueKind(), E->getCastKind(),
        SubExpr, &BasePath, TInfo,
        Importer.Import(CCE->getLParenLoc()),
        Importer.Import(CCE->getRParenLoc()));
}
/* more cases*/


As the AST contains circular references and many nodes require an ASTContext
for construction, i'm wondering how feasible it would be to implement the following interface for each Decl?

 virtual Decl* clone(
   ASTContext* newContext, 
   DenseMap<Decl*, Decl*> ImportedDeclsCache = {}
 );






Sent with Mailtrack


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev