[PATCH] D65573: Add User docs for ASTImporter
Gabor Marton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 2 07:25:12 PDT 2019
martong marked 23 inline comments as done.
martong added a comment.
Thanks for the review!
Comment at: clang/docs/LibASTImporter.rst:19
+``ASTContext`` holds long-lived AST nodes (such as types and decls) that can be referred to throughout the semantic analysis of a file.
+There are cases when we would like to work with more than one ``ASTContext``.
+For example, we'd like to parse multiple different files inside the same Clang tool.
> Stylistic note: usually LLVM documentation is written avoiding "we". So for example:
> In some cases it is preferable to work with more than one ``ASTContext``, for example when parsing multiple files inside one Clang-based tool.
In one hand, I accept that maybe the word "we" is overused in this document.
I'll try to rephrase those sentences where it feels awkward.
On the other hand, I feel if all occurrences of "we" were replaced then the document would become filled with passive voice. That would make it too formal and would provide harder reading in my opinion.
Also, other AST related documentation uses "we" quite frequently. E.g. https://clang.llvm.org/docs/LibASTMatchersTutorial.html has 42 matches of "we".
l have rephrased those sentences which you explicitly mentioned, please add comments to those sentences where you still feel the use of "we" inappropriate.
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
I changed to Adrian's suggestion.
Comment at: clang/docs/LibASTImporter.rst:110
+Now we create the Importer and do the import:
> Maybe helpful to link to the [Matching the Clang AST](https://clang.llvm.org/docs/LibASTMatchers.html) and [AST Matcher Reference](https://clang.llvm.org/docs/LibASTMatchersReference.html)
Ok, I've added them to the beginning of the doc.
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 ...
"If these definitions differ, then we have a name conflict, in C++ it is known as ODR (one definition rule) violation."
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.
I changed to use past perfect as well to emphasize that the node had been created before the error was recognized.
"We may recognize an error during the import of a dependent node. However, by that time, we had already created the dependant."
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits