[PATCH] D66336: [ASTImporter] Add development internals docs

[Gabor Marton via Phabricator via cfe-commits]

martong marked 6 inline comments as done.
martong added a comment.

Thanks for the review!



================
Comment at: clang/docs/InternalsManual.rst:1470
+*templated* class (the ``CXXRecordDecl``) of a ``ClassTemplateDecl`` with
+``ClassTemplateDecl::getTemplatedDec()``. And we can get back a pointer of the
+"described" class template from the *templated* class:
----------------
a_sidorin wrote:
> TemplatedDec_l_?
Could you please elaborate, I don't get the meaning of the comment.


================
Comment at: clang/docs/InternalsManual.rst:1528
+
+When we compare two classes or enums and one of them is incomplete or has
+unloaded external lexical declarations then we cannot descend to compare their
----------------
a_sidorin wrote:
> I think we can extend this to point that import of all named declarations is preceded by name lookup.
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".



================
Comment at: clang/docs/InternalsManual.rst:1660
+context. Consequently, calling a declaration's ``::Create()`` function directly
+would lead to errors, please don't do that!
+
----------------
balazske wrote:
> 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`.
Ok, I added that, but rephrased to connect smoother to the context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66336