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

[Serge Pavlov via cfe-commits]

sepavloff added a comment.

I still cannot build project with your changes, now compiler cannot find symbol `hasSubStmt`. When committing the change please make sure that all prerequisites are committed and unit tests run successfully.

I would recommend you to take tests from http://reviews.llvm.org/D14224 for literals. They check imported values as well, not only syntax structure. It is however only a recommendation.

There is no tests for `InjectedClassNameType`, `TemplateTypeParmType`, `GCCAsmStmt`, `VAArgExpr` and `CXXBoolLiteralExpr`. Probably these nodes may be moved to next code change?

Provided that the aforementioned nodes get tests of are moved to another change, the patch looks good for me. I do not have authority to approve patches for commit, you need to ask someone with proper rights for the approval.

Thank you!


================
Comment at: lib/AST/ASTImporter.cpp:1827-1828
@@ +1826,4 @@
+
+  // FIXME: ASTContext::getInjectedClassNameType is not suitable for AST reading
+  // See comments in its definition for details
+  // return Importer.getToContext().getInjectedClassNameType(D, InjType);
----------------
Definition of `ASTContext::getInjectedClassNameType` does not contain comments that would explain why it is unsuitable, so this comment is confusing. Probably you wanted to reference `ASTReader::readTypeRecord`, it contains similar statement.

================
Comment at: lib/AST/ASTImporter.cpp:4379
@@ +4378,3 @@
+
+  // Resolve possible cyclic import
+  if (Decl *AlreadyImported = Importer.GetAlreadyImportedOrNull(D))
----------------
Comments should be proper English sentences. In this case ending period is missed.

================
Comment at: lib/AST/ASTImporter.cpp:4730
@@ +4729,3 @@
+  SmallVector<IdentifierInfo *, 4> Names;
+  for (unsigned i = 0, e = S->getNumOutputs(); i != e; i++) {
+    IdentifierInfo *ToII = Importer.Import(S->getOutputIdentifier(i));
----------------
According to LLVM coding style ([[ http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly ]]) variable names 'should be camel case, and start with an upper case letter', so `i` -> `I`, `e` -> `E`.

================
Comment at: unittests/AST/ASTImporterTest.cpp:117
@@ +116,3 @@
+                                   hasType(
+                                     asString("const char [4]")))))))));
+  EXPECT_TRUE(testImport("void declToImport() { L\"foo\"; }",
----------------
Disadvantage of using matchers, apart of complexity, is that they check only syntax structure. This test does not check if imported literal has proper value.


Repository:
  rL LLVM

http://reviews.llvm.org/D14286