<div dir="ltr"><div>> <span style="font-size:12.8px">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</span></div><div><br></div><div>>> <span style="font-size:12.8px">There should be no infinite loops in ASTImporter as they are broken by GetAlreadyImportedOrNull() method.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">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.</span></div><div><span style="font-size:12.8px">I also appreciate your recent patch fixing one of those issues.</span></div><div><br></div><div>> <span style="font-size:12.8px">It is also interesting for me what will be the difference between clone() methods and methods of ASTImporter. Do you have any example?</span><br></div><div><span style="font-size:12.8px">My main concerns are maintainability, correctness and encapsulation.</span></div><div><span style="font-size:12.8px"><br></span></div><div>Decl's are arranged in a class-hierarchy; they have additional data and are primary used through the Decl interface.<br></div><div>If we want to clone/import a concrete Decl we need:</div><div>- to query for the underlying type </div><div>- downcast</div><div>- clone its containing data.</div><div><br></div><div>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.<br></div><div><br></div><div>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. <br></div><div><br></div><br><div class="mt-signature"><a href="https://mailtrack.io/trace/link/f40fe287c68326e0f6661b941ea6b5ecedc2091e?url=https%3A%2F%2Fmailtrack.io%2F&signature=08c1bcae30ed39e9" class="gmail-mt-signature-logo" style="text-decoration:none"><img src="https://s3-eu-west-1.amazonaws.com/mailtrack-crx/icon-signature.png" width="16" height="14"> </a><font color="#999999" class="mt-signature-text">Sent with <a href="https://mailtrack.io/install?source=signature&lang=en&referral=gaetano.checinski@gmail.com&idSignature=22" class="mt-install">Mailtrack</a></font></div><img width="0" height="0" class="mailtrack-img" src="https://mailtrack.io/trace/mail/fa7c4c9112a802eb7d7b27aed22dacf69836e129.png?u=931501"></div><div class="gmail_extra"><br><div class="gmail_quote">2017-01-18 17:32 GMT+00:00 Aleksei Sidorin via cfe-dev <span dir="ltr"><<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div class="m_6534975331693431624moz-cite-prefix">CC'ing cfe-dev again.<br>
<br>
There should be no infinite loops in ASTImporter as they are
broken by GetAlreadyImportedOrNull() method.<br>
It is also interesting for me what will be the difference between
clone() methods and methods of ASTImporter. Do you have any
example?<br>
<br>
<br>
18.01.2017 20:24, Gaetano Checinski пишет:<br>
</div><div><div class="h5">
<blockquote type="cite">
<div dir="ltr">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.
<div>This is why my proposed signature for the clone contains
not only an <font face="monospace, monospace">ASTContext</font>
to clone into but also a `<span style="font-family:monospace,monospace;font-size:12.8px">DenseMap<Decl*,
Decl*>& ImportedDeclsCache` </span><span style="font-size:12.8px"><font face="arial, helvetica,
sans-serif">to keep track of those already imported Decls.</font></span></div>
<div><br>
</div>
<img class="m_6534975331693431624mailtrack-img" src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" height="0" width="0"></div>
<div class="gmail_extra"><br>
<div class="gmail_quote">2017-01-18 17:16 GMT+00:00 Aleksei
Sidorin <span dir="ltr"><<a href="mailto:a.sidorin@samsung.com" target="_blank">a.sidorin@samsung.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div class="m_6534975331693431624m_5220777893620003423moz-cite-prefix">Hello
Gaetano,<br>
<br>
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.<br>
<br>
I was thinking about "clone" approach some time ago, but
the question how to handle complex dependencies is still
open here.<br>
<br>
<br>
<br>
18.01.2017 19:55, Gaetano Checinski via cfe-dev пишет:<br>
</div>
<blockquote type="cite">
<div>
<div class="m_6534975331693431624h5">
<div dir="ltr">I was wondering if anybody spend some
efforts in implementing a clone Method for all
ASTNodes?
<div><br>
</div>
<div>It seems to me, that the current approach
implemented in the ASTImporter is error-prone
and hard to maintain.</div>
<div><br>
</div>
<div>Currently it looks to me that Decls / Stmt's
are being probed for their type and then
downcasted, eg.:<br>
<br>
</div>
<blockquote style="margin:0 0 0 40px;border:none;padding:0px">
<div><font face="monospace, monospace">/* ASTNodeImporter::VisitExpli<wbr>citCastExpr
*/</font></div>
<div><font face="monospace, monospace">/*
ASTImporter.cpp:6060 */</font></div>
<div><font face="monospace, monospace">switch
(E->getStmtClass()) {</font></div>
<div><font face="monospace, monospace"> case
Stmt::CStyleCastExprClass: {</font></div>
<div><font face="monospace, monospace">
CStyleCastExpr *CCE =
cast<CStyleCastExpr>(E);</font></div>
<div><font face="monospace, monospace">
return CStyleCastExpr::Create(Importe<wbr>r.getToContext(),
T,</font></div>
<div><font face="monospace, monospace">
E->getValueKind(), E->getCastKind(),</font></div>
<div><font face="monospace, monospace">
SubExpr, &BasePath, TInfo,</font></div>
<div><font face="monospace, monospace">
Importer.Import(CCE->getLParen<wbr>Loc()),</font></div>
<div><font face="monospace, monospace">
Importer.Import(CCE->getRParen<wbr>Loc()));</font></div>
<div><font face="monospace, monospace">}</font></div>
<div><font face="monospace, monospace">/* more
cases*/</font></div>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>As the AST contains circular references and
many nodes require an ASTContext</div>
<div>for construction, i'm wondering how feasible
it would be to implement the following interface
for each Decl?</div>
<div><br>
</div>
<blockquote style="margin:0 0 0 40px;border:none;padding:0px">
<div><font face="monospace, monospace"> virtual
Decl* clone(</font></div>
<div><font face="monospace, monospace">
ASTContext* newContext, </font></div>
<div><font face="monospace, monospace">
DenseMap<Decl*, Decl*>
ImportedDeclsCache = {}</font></div>
<div><font face="monospace, monospace"> );</font></div>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
<br>
<br>
<div class="m_6534975331693431624m_5220777893620003423mt-signature"><a href="https://mailtrack.io/trace/link/b910ce4d02f08facbacacd7152c370d845d81397?url=https%3A%2F%2Fmailtrack.io%2F&signature=e6da9dfa433d9e41" class="m_6534975331693431624m_5220777893620003423gmail-mt-signature-logo" style="text-decoration:none" target="_blank"><img src="https://s3-eu-west-1.amazonaws.com/mailtrack-crx/icon-signature.png" height="14" width="16"> </a><font class="m_6534975331693431624m_5220777893620003423mt-signature-text" color="#999999">Sent with <a href="https://mailtrack.io/install?source=signature&lang=en&referral=gaetano.checinski@gmail.com&idSignature=22" class="m_6534975331693431624m_5220777893620003423mt-install" target="_blank">Mailtrack</a></font></div>
</div>
<img class="m_6534975331693431624m_5220777893620003423mailtrack-img" height="0" width="0"></div>
<br>
<fieldset class="m_6534975331693431624m_5220777893620003423mimeAttachmentHeader"></fieldset>
<br>
</div>
</div>
<pre>______________________________<wbr>_________________
cfe-dev mailing list
<a class="m_6534975331693431624m_5220777893620003423moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>
<a class="m_6534975331693431624m_5220777893620003423moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-dev</a><span class="m_6534975331693431624HOEnZb"><font color="#888888">
</font></span></pre><span class="m_6534975331693431624HOEnZb"><font color="#888888">
</font></span></blockquote><span class="m_6534975331693431624HOEnZb"><font color="#888888">
<p>
</p>
<pre class="m_6534975331693431624m_5220777893620003423moz-signature" cols="72">--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics
</pre>
</font></span></div>
</blockquote></div>
</div>
</blockquote>
<p>
</p><pre class="m_6534975331693431624moz-signature" cols="72">--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics
</pre></div></div></div>
<br>______________________________<wbr>_________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div>