[PATCH] D65573: Add User docs for ASTImporter

[Gabor Marton via Phabricator via cfe-commits]

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.
----------------
aprantl wrote:
> 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?
----------------
gamesh411 wrote:
> 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:
+
----------------
shafik wrote:
> 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:
----------------
gamesh411 wrote:
> 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 ...
Changed to 
"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.
----------------
gamesh411 wrote:
> 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."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65573