This document includes the description of the ASTImporter from the user/client
perspective.
A subsequent patch will describe the development internals.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Thanks, the extra documentation is highly appreciated!
clang/docs/LibASTImporter.rst | ||
---|---|---|
19 ↗ | (On Diff #212798) | 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. |
33 ↗ | (On Diff #212798) | Importing ... copies that node ... |
Thank you for writing this up! I just have a few minor comments.
clang/docs/LibASTImporter.rst | ||
---|---|---|
110 ↗ | (On Diff #212798) | Maybe helpful to link to the Matching the Clang AST and AST Matcher Reference |
283 ↗ | (On Diff #212798) | Note ODR is a C++ concept C does not have ODR |
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!
clang/docs/LibASTImporter.rst | ||
---|---|---|
26 ↗ | (On Diff #212798) | I'd prefer: |
32 ↗ | (On Diff #212798) | I'd put a comma here: |
34 ↗ | (On Diff #212798) | Leave the just, or rephrase: |
42 ↗ | (On Diff #212798) | I'd put a comma here: |
74 ↗ | (On Diff #212798) | I'd put a comma here: |
154 ↗ | (On Diff #212798) | Leave the 'And' |
190 ↗ | (On Diff #212798) | I'd put a comma here: |
272 ↗ | (On Diff #212798) | Leave the 'And' |
283 ↗ | (On Diff #212798) | I'd put a comma here: |
347 ↗ | (On Diff #212798) | I'd use simple present here: |
388 ↗ | (On Diff #212798) | Not a native, but this stood out: |
402 ↗ | (On Diff #212798) | I'd switch these: |
410 ↗ | (On Diff #212798) | I'd phrase it like this: |
476 ↗ | (On Diff #212798) | I'd put a comma here: |
Thanks for the review!
clang/docs/LibASTImporter.rst | ||
---|---|---|
19 ↗ | (On Diff #212798) | In one hand, I accept that maybe the word "we" is overused in this document. 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. l have rephrased those sentences which you explicitly mentioned, please add comments to those sentences where you still feel the use of "we" inappropriate. |
32 ↗ | (On Diff #212798) | I changed to Adrian's suggestion. |
110 ↗ | (On Diff #212798) | Ok, I've added them to the beginning of the doc. |
283 ↗ | (On Diff #212798) | Changed to |
410 ↗ | (On Diff #212798) | I changed to use past perfect as well to emphasize that the node had been created before the error was recognized. |
Thanks Alexei for the review! I commited a fix for the typos.
cfe/trunk/docs/LibASTImporter.rst | ||
---|---|---|
215 | Indeed... Still, I did not change that to keep consistency with other parts of the examples. |
We can avoid const_cast if we change the example function signature to const Node *.