[PATCH] D14286: ASTImporter: expressions, pt.1

[Serge Pavlov via cfe-commits]

2015-11-06 20:28 GMT+06:00 Aleksei Sidorin <a.sidorin at samsung.com>:

> a.sidorin marked 7 inline comments as done.
> a.sidorin added a comment.
>
> Thank you for your comments. I leaved some replies and will update
> revision.
> Something about lacking tests.
>
> 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?
>

There is implementation of FunctionDecl import, it definitely can import
function body, tests ASTMerge/Inputs/{body1,body2} check this. In
http://reviews.llvm.org/D14224 function bodies are used to check that some
statement nodes are imported successfully. You may take necessary tests
from there.


> 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.
>
>
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.


>
> ================
> Comment at: lib/AST/ASTImporter.cpp:35
> @@ +34,3 @@
> +    void ImportMultipleItems(IIter Ibegin, IIter Iend, OIter Obegin) {
> +      ASTImporter &_Importer = Importer;
> +      std::transform(Ibegin, Iend, Obegin,
> ----------------
> sepavloff wrote:
> > 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.
> Ouch. I was just trying to unify this with the code already existing in
> ASTImporter. I'll rename this.
> // TODO: refactor std::transform usage in ASTImporter.
>
> ================
> Comment at: lib/AST/ASTImporter.cpp:48
> @@ +47,3 @@
> +    template<typename IIter, typename OIter>
> +    bool checkPossibleNull(IIter Ibegin, IIter Iend, OIter Obegin) {
> +      for (; Ibegin != Iend; Ibegin++, Obegin++)
> ----------------
> sepavloff wrote:
> > This function is used in one place only, maybe inline its body in that
> place?
> I'll use it in later patches so I'd prefer to keep it.
>
> ================
> Comment at: lib/AST/ASTImporter.cpp:4655
> @@ +4654,3 @@
> +  for (unsigned i = 0, e = S->getNumClobbers(); i != e; i++) {
> +    StringLiteral *Clobber = cast_or_null<StringLiteral>(
> +          Importer.Import(S->getClobberStringLiteral(i)));
> ----------------
> sepavloff wrote:
> > Again, clobber cannot be null, maybe `cast` instead of `cast_or_null`?
> This guard is here because the return result of import may be null. This
> cast_or_null (and some others) handles such cases.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D14286
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151109/1d2799a5/attachment.html>