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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
