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

[Serge Pavlov via cfe-commits]

sepavloff added a comment.

The fix must contain tests.


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

================
Comment at: lib/AST/ASTImporter.cpp:43
@@ +42,3 @@
+    template<typename IIter>
+    bool checkNull(IIter Ibegin, IIter Iend) {
+      return std::find(Ibegin, Iend, nullptr) == Iend;
----------------
I would propose more descriptive name like `containsNullptr` and move it into anonymous namespace, or maybe eliminated this function at all, and use std::find directly.

================
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++)
----------------
This function is used in one place only, maybe inline its body in that place?

================
Comment at: lib/AST/ASTImporter.cpp:4642
@@ +4641,3 @@
+    IdentifierInfo *ToII = Importer.Import(S->getOutputIdentifier(i));
+    if (!ToII && S->getOutputIdentifier(i))
+      return nullptr;
----------------
Check for `S->getOutputIdentifier(i)` seems redundant, output name cannot be omitted in asm statement.

================
Comment at: lib/AST/ASTImporter.cpp:4648
@@ +4647,3 @@
+    IdentifierInfo *ToII = Importer.Import(S->getInputIdentifier(i));
+    if (!ToII && S->getInputIdentifier(i))
+      return nullptr;
----------------
Same as above.

================
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)));
----------------
Again, clobber cannot be null, maybe `cast` instead of `cast_or_null`?

================
Comment at: lib/AST/ASTImporter.cpp:4664
@@ +4663,3 @@
+  for (unsigned i = 0, e = S->getNumOutputs(); i != e; i++) {
+    StringLiteral *Output = cast_or_null<StringLiteral>(
+          Importer.Import(S->getOutputConstraintLiteral(i)));
----------------
`cast_or_null` -> `cast`?

================
Comment at: lib/AST/ASTImporter.cpp:4672
@@ +4671,3 @@
+  for (unsigned i = 0, e = S->getNumInputs(); i != e; i++) {
+    StringLiteral *Input = cast_or_null<StringLiteral>(
+          Importer.Import(S->getInputConstraintLiteral(i)));
----------------
`cast_or_null` -> `cast`?

================
Comment at: lib/AST/ASTImporter.cpp:4693
@@ +4692,3 @@
+
+  StringLiteral *AsmStr = cast_or_null<StringLiteral>(
+        Importer.Import(S->getAsmString()));
----------------
`cast_or_null` -> `cast`?

================
Comment at: lib/AST/ASTImporter.cpp:5248
@@ +5247,3 @@
+    Expr *Filter = Importer.Import(ILE->getArrayFiller());
+    if (!Filter && ILE->getArrayFiller())
+      return nullptr;
----------------
Check for `ILE->getArrayFiller()` is useless, as in this branch `ILE->hasArrayFiller()` is true.

================
Comment at: lib/AST/ASTImporter.cpp:5253-5257
@@ +5252,7 @@
+
+  FieldDecl *ToFD = cast_or_null<FieldDecl>(
+        Importer.Import(ILE->getInitializedFieldInUnion()));
+  if (!ToFD && ILE->getInitializedFieldInUnion())
+    return nullptr;
+  To->setInitializedFieldInUnion(ToFD);
+
----------------
Call to `setInitializedFieldInUnion` will always set type of respective union to declaration, so if it contained an expression, it will be lost. The safer solution is to check the kind of the source union and import either declaration or expression depending on the result.

================
Comment at: lib/AST/ASTImporter.cpp:5301
@@ +5300,3 @@
+  // List elements from the second, the first is Init itself
+  for (unsigned i = 1, e = DIE->getNumSubExprs(); i < e; i++) {
+    if (Expr *Arg = cast_or_null<Expr>(Importer.Import(DIE->getSubExpr(i))))
----------------
According to LLVM coding standard variable names must start with upper case letter, so `i` -> `I`, `e`->`E`.


Repository:
  rL LLVM

http://reviews.llvm.org/D14286