Page MenuHomePhabricator

Add User docs for ASTImporter
ClosedPublic

Authored by martong on Aug 1 2019, 6:27 AM.

Details

Summary

This document includes the description of the ASTImporter from the user/client
perspective.
A subsequent patch will describe the development internals.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Aug 1 2019, 6:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
martong edited reviewers, added: dkrupp; removed: a.sidorin.Aug 1 2019, 9:24 AM
aprantl added a subscriber: aprantl.Aug 1 2019, 9:58 AM

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:
... translation unit (TU). This way, the analysis can ...

32 ↗(On Diff #212798)

I'd put a comma here:
By importing one AST node, we copy

34 ↗(On Diff #212798)

Leave the just, or rephrase:
Isn't it enough to insert ...

42 ↗(On Diff #212798)

I'd put a comma here:
... existing definitions, and ...

74 ↗(On Diff #212798)

I'd put a comma here:
... identifier name, and their ...

154 ↗(On Diff #212798)

Leave the 'And'

190 ↗(On Diff #212798)

I'd put a comma here:
... without definition, and we have ...

272 ↗(On Diff #212798)

Leave the 'And'

283 ↗(On Diff #212798)

I'd put a comma here:
If these definitions differ, then we ...

347 ↗(On Diff #212798)

I'd use simple present here:
The AST will not contain the ... -> The AST does not contain the ...

388 ↗(On Diff #212798)

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.

402 ↗(On Diff #212798)

I'd switch these:
set also -> also set

410 ↗(On Diff #212798)

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.

476 ↗(On Diff #212798)

I'd put a comma here:
If we take a look at the AST, then we can ...

martong marked 23 inline comments as done.Aug 2 2019, 7:22 AM

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.
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.

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
"If these definitions differ, then we have a name conflict, in C++ it is known as ODR (one definition rule) violation."

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.
"We may recognize an error during the import of a dependent node. However, by that time, we had already created the dependant."

martong updated this revision to Diff 213046.Aug 2 2019, 7:22 AM
martong marked 5 inline comments as done.
  • Address comments
martong updated this revision to Diff 213332.Aug 5 2019, 5:31 AM
  • Add description for -ast-merge
gamesh411 accepted this revision.Aug 6 2019, 1:31 AM
This revision is now accepted and ready to land.Aug 6 2019, 1:31 AM
Closed by commit rL368009: Add User docs for ASTImporter (authored by martong, committed by ). · Explain WhyAug 6 2019, 2:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 2:53 AM

That's incredible. Thank you!

cfe/trunk/docs/LibASTImporter.rst
215

We can avoid const_cast if we change the example function signature to const Node *.

258

CMakeLists.txt

557

let's (small letter).

martong marked 3 inline comments as done.Aug 16 2019, 5:22 AM

That's incredible. Thank you!

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.