[PATCH] D65573: Add User docs for ASTImporter

Fri Aug 2 05:28:14 PDT 2019

gamesh411 added a comment.

Lovely documentation with practical use-cases!
I left a few inline remarks.
Also wouldn't it be nice to have a section which introduces the `-ast-merge` command-line option. It would be helpful to see an example where a PCH is dumped and merged into another TU. You could mention, that this can be used to debug ASTImporter functionality.
Cheers!

================
Comment at: clang/docs/LibASTImporter.rst:26
+Existing clients of the ``ASTImporter`` library are Cross Translation Unit (CTU) static analysis and the LLDB expression parser.
+CTU static analysis imports a definition of a function if its definition is found in another translation unit (TU), this way the analysis can breach out from the single TU limitation.
+LLDB's ``expr`` command parses a user-defined expression, creates an ``ASTContext`` for that and then imports the missing definitions from the AST what we got from the debug information (DWARF, etc).
----------------
I'd prefer:
... translation unit (TU). This way, the analysis can ...

================
Comment at: clang/docs/LibASTImporter.rst:32
+
+By importing one AST node we copy that node into the destination ``ASTContext``.
+Why do we have to copy the node?
----------------
I'd put a comma here:
By importing one AST node,  we copy

================
Comment at: clang/docs/LibASTImporter.rst:34
+Why do we have to copy the node?
+Isn't enough just to insert the pointer to that node into the destination context?
+One reason is that the "from" context may outlive the "to" context.
----------------
Leave the just, or rephrase:
Isn't it enough to insert ...

================
Comment at: clang/docs/LibASTImporter.rst:42
+For instance, if there is a class definition with the same name in both translation units, but one of the definition contains a different number of fields.
+So, we look up existing definitions and then we check the structural equivalency on those nodes.
+The following pseudo-code demonstrates the basics of the import mechanism:
----------------
I'd put a comma here:
... existing definitions, and ...

================
Comment at: clang/docs/LibASTImporter.rst:74
+- record types and all their fields in order of their definition have the same identifier names and structurally equivalent types,
+- variable or function declarations and they have the same identifier name and their types are structurally equivalent.
+
----------------
I'd put a comma here:
... identifier name, and their ...

================
Comment at: clang/docs/LibASTImporter.rst:154
+We'd like to get the members too, so, we use ``ImportDefinition`` to copy the whole definition of ``MyClass`` into the "to" context.
+And then we dump the AST again.
+
----------------
Leave the 'And'

================
Comment at: clang/docs/LibASTImporter.rst:190
+With **normal import**, all dependent declarations are imported normally.
+However, with minimal import, the dependent Decls are imported without definition and we have to import their definition for each if we later need that.
+
----------------
I'd put a comma here:
... without definition, and we have ...

================
Comment at: clang/docs/LibASTImporter.rst:272
+
+And then we can build and execute the new tool.
+
----------------
Leave the 'And'

================
Comment at: clang/docs/LibASTImporter.rst:283
+However, there may be cases when both of the contexts have a definition for a given symbol.
+If these definitions differ then we have a name conflict, otherwise known as ODR (one definition rule) violation.
+Let's modify the previous tool we had written and try to import a ``ClassTemplateSpecializationDecl`` with a conflicting definition:
----------------
shafik wrote:
> Note ODR is a C++ concept C does not have ODR
I'd put a comma here:
If these definitions differ, then we ...

================
Comment at: clang/docs/LibASTImporter.rst:347
+Since we could not import the specified declaration (``From``), we get an error in the return value.
+The AST will not contain the conflicting definition, so we are left with the original AST.
+
----------------
I'd use simple present here:
The AST will not contain the ... -> The AST does not contain the ...

================
Comment at: clang/docs/LibASTImporter.rst:388
+
+In this case we can see that an error is associated (``getImportDeclErrorIfAny``) for the specialization also, not just for the field:
+
----------------
Not a native, but this stood out:
In this case, we can see that an error is associated (``getImportDeclErrorIfAny``) **with/to** the specialization also, not just **with/to** the field.

================
Comment at: clang/docs/LibASTImporter.rst:402
+    assert(Importer.getImportDeclErrorIfAny(FromSpec));
+    // Btw, the error is set also for the FieldDecl.
+    assert(Importer.getImportDeclErrorIfAny(From));
----------------
I'd switch these:
set also -> also set

================
Comment at: clang/docs/LibASTImporter.rst:410
+
+There are cases when during the import of a dependent node we recognize an error, however, by that time we already created the dependant.
+In these cases we do not remove the existing erroneous node from the "to" context, rather we associate an error to that node.
----------------
I'd phrase it like this:
We may recognize an error during the import of a dependent node. However, by that time, we have already created the dependant.

================
Comment at: clang/docs/LibASTImporter.rst:476
+
+If we take a look at the AST then we can see that the Decl with the definition is created, but the field is missing.
+
----------------
I'd put a comma here:
If we take a look at the AST, then we can ...

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65573/new/

https://reviews.llvm.org/D65573