Page MenuHomePhabricator

[ASTImporter] Add development internals docs
ClosedPublic

Authored by martong on Aug 16 2019, 2:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Aug 16 2019, 2:53 AM
Herald added a project: Restricted Project. · View Herald Transcript

Hello Gabor,
This doc will become extremely useful for new developers. Thank you for dumping this sacred knowledge!

clang/docs/InternalsManual.rst
1470 ↗(On Diff #215553)

TemplatedDec_l_?

1528 ↗(On Diff #215553)

I think we can extend this to point that import of all named declarations is preceded by name lookup.

balazske added inline comments.Aug 21 2019, 1:00 AM
clang/docs/InternalsManual.rst
1569 ↗(On Diff #215553)

It can be mentioned that the structural equivalency identifies the chains with the canonical decl, that becomes different for independent chains (in this case even for the same type, this is why these are seen as different).

1585 ↗(On Diff #215553)

I do not like "we will" here, who will really do this work? (It can be the reader.)

1660 ↗(On Diff #215553)

We can mention that there is still the probability of having an infinite import recursion if things are imported from each other in wrong way. (From a visitor for A import of B is requested before create of node A and the same in visitor for B. The new A node is not yet created and not in ImportedDecls when import of B is reached.) The best is to have no import of other nodes in a visitor for A before create of node A. Or if really needed (for example in the template case) only for type B that does not the same thing with type A.

martong marked 6 inline comments as done.Aug 22 2019, 4:04 AM

Thanks for the review!

clang/docs/InternalsManual.rst
1470 ↗(On Diff #215553)

Could you please elaborate, I don't get the meaning of the comment.

1528 ↗(On Diff #215553)

I would not put that here, because in this section we describe the structural equivalency, which can be used independently from the import mechanism.

Plus, we mention that explicitly in the prerequisite User document of ASTImporter ( LibASTImporter.rst)
We also mention it later, in this section: "Lookup Problems".

1660 ↗(On Diff #215553)

Ok, I added that, but rephrased to connect smoother to the context.

martong updated this revision to Diff 216579.Aug 22 2019, 4:04 AM
martong marked an inline comment as done.
  • Address review comments
a_sidorin added inline comments.Aug 24 2019, 9:33 AM
clang/docs/InternalsManual.rst
1470 ↗(On Diff #215553)

Sorry, I mean, you probably meant ClassTemplateDecl::getTemplatedDecl but typed getTemplatedDec() (missing 'l' at the end).

martong updated this revision to Diff 217171.Aug 26 2019, 8:47 AM
  • Fix typo: getTemplatedDec -> getTemplatedDecl
martong marked 2 inline comments as done.Aug 26 2019, 8:48 AM
martong added inline comments.
clang/docs/InternalsManual.rst
1470 ↗(On Diff #215553)

Ok, thanks now I see what you meant. Fixed it. Thanks.

martong marked 2 inline comments as done.Aug 26 2019, 8:48 AM
a_sidorin accepted this revision.Sep 8 2019, 3:45 AM

Thank you!

This revision is now accepted and ready to land.Sep 8 2019, 3:45 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 4:20 AM