<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">2015-11-06 20:28 GMT+06: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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">a.sidorin marked 7 inline comments as done.<br>
a.sidorin added a comment.<br>
<br>
Thank you for your comments. I leaved some replies and will update revision.<br>
Something about lacking tests.<br>
<br>
1. We cannot check if expression import is correct until we implement FunctionDecl body import. I was going to upstream Decl-related parts later but maybe it is better to include this small case in the first patch. What do you think?<br></blockquote><div><br></div><div>There is implementation of FunctionDecl import, it definitely can import function body, tests ASTMerge/Inputs/{body1,body2} check this. In <a href="http://reviews.llvm.org/D14224">http://reviews.llvm.org/D14224</a> function bodies are used to check that some statement nodes are imported successfully. You may take necessary tests from there.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
2. What is the best way to test import of statements? Currently I have no idea how to do this (except of ast-dump). Any suggestions are welcome.<br>
<span class=""><br></span></blockquote><div><br></div><div>IMHO ast-dump is not the best way to check this. It does not check if import was right. Many tests in ASTMerge just check that Importer handles particular node, as a first step it looks sufficient. But you can go further and define constexpr function in imported module, which uses the node being tested. Then in the importing module you call this constexpr function in static_assert. Not all nodes can be tested in this way, but for those that can (for instance, ConditionalOperator) it would be a good test.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">
<br>
================<br>
Comment at: lib/AST/ASTImporter.cpp:35<br>
@@ +34,3 @@<br>
+    void ImportMultipleItems(IIter Ibegin, IIter Iend, OIter Obegin) {<br>
+      ASTImporter &_Importer = Importer;<br>
+      std::transform(Ibegin, Iend, Obegin,<br>
----------------<br>
</span><span class="">sepavloff wrote:<br>
> A name started with underscore is a reserved identifier (see C++ standard, [global.names]), so it is better to use something more neutral, like //TheImporter// or //Imp// or something else.<br>
</span>Ouch. I was just trying to unify this with the code already existing in ASTImporter. I'll rename this.<br>
// TODO: refactor std::transform usage in ASTImporter.<br>
<span class=""><br>
================<br>
Comment at: lib/AST/ASTImporter.cpp:48<br>
@@ +47,3 @@<br>
+    template<typename IIter, typename OIter><br>
+    bool checkPossibleNull(IIter Ibegin, IIter Iend, OIter Obegin) {<br>
+      for (; Ibegin != Iend; Ibegin++, Obegin++)<br>
----------------<br>
</span><span class="">sepavloff wrote:<br>
> This function is used in one place only, maybe inline its body in that place?<br>
</span>I'll use it in later patches so I'd prefer to keep it.<br>
<span class=""><br>
================<br>
Comment at: lib/AST/ASTImporter.cpp:4655<br>
@@ +4654,3 @@<br>
+  for (unsigned i = 0, e = S->getNumClobbers(); i != e; i++) {<br>
+    StringLiteral *Clobber = cast_or_null<StringLiteral>(<br>
+          Importer.Import(S->getClobberStringLiteral(i)));<br>
----------------<br>
</span><span class="">sepavloff wrote:<br>
> Again, clobber cannot be null, maybe `cast` instead of `cast_or_null`?<br>
</span>This guard is here because the return result of import may be null. This cast_or_null (and some others) handles such cases.<br>
<div class=""><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D14286" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14286</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div></div>