[cfe-dev] implementing clone for ASTNodes

[Gaetano Checinski 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.



<https://mailtrack.io/trace/link/f40fe287c68326e0f6661b941ea6b5ecedc2091e?url=https%3A%2F%2Fmailtrack.io%2F&signature=08c1bcae30ed39e9>Sent
with Mailtrack
<https://mailtrack.io/install?source=signature&lang=en&referral=gaetano.checinski@gmail.com&idSignature=22>

2017-01-18 17:32 GMT+00:00 Aleksei Sidorin via cfe-dev <
cfe-dev at lists.llvm.org>:

> 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 <a.sidorin at samsung.com>:
>
>> 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 = {}
>>  );
>>
>>
>>
>>
>>
>>
>>
>>
>> <https://mailtrack.io/trace/link/b910ce4d02f08facbacacd7152c370d845d81397?url=https%3A%2F%2Fmailtrack.io%2F&signature=e6da9dfa433d9e41>Sent
>> with Mailtrack
>> <https://mailtrack.io/install?source=signature&lang=en&referral=gaetano.checinski@gmail.com&idSignature=22>
>>
>>
>> _______________________________________________
>> cfe-dev mailing listcfe-dev at lists.llvm.orghttp://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
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170201/98cb2143/attachment.html>