This is an archive of the discontinued LLVM Phabricator instance.

[clang-doc] Add template support
ClosedPublic

Authored by brettw on Dec 1 2022, 3:57 PM.

Details

Summary

Reads template information from the AST and adds template parameters and specialization information to the corresponding clang-doc structures.

Add a "QualName" to the Reference struct which includes the full qualified type name. The Reference object represents a link in the HTML/MD generators so is based on the unqualified name. But this does not encode C-V qualifiers or template information that decorate the name. The new QualName member encodes all of this information and also makes it easier for the generators or downsteam YAML consumers to generate the full name (before they had to process the "Path").

In test code that was changed, remove made-up paths to built-in types like "int". In addition to slightnly cleaning up the code, these types do not have paths in real execution, and generating incorrect references to nonexistant data may complicate future changes in the generators.

Diff Detail

Event Timeline

brettw created this revision.Dec 1 2022, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 3:57 PM
brettw requested review of this revision.Dec 1 2022, 3:57 PM
brettw updated this revision to Diff 479772.Dec 2 2022, 4:12 PM
brettw edited the summary of this revision. (Show Details)Dec 5 2022, 2:37 PM
brettw updated this revision to Diff 480251.Dec 5 2022, 2:40 PM
brettw added a reviewer: paulkirth.

Can we also update the end-to-end tests with some relevant test cases? I think that's especially important, given that I'm not sure the unittests are running as part of the normal test suite.

Overall this is mostly LGTM with some minor comments/questions.

clang-tools-extra/clang-doc/BitcodeReader.cpp
377

Do you expect to add handling for more cases here? If so, a comment with FIXME or TODO is appropriate. If not, then I'm not sure a switch is a better choice than a branch ...

clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
631

I may have missed these cases in the test file, so if they exist just ignore me.

  • a template w/ no concrete types e.g., template<typename T> void GetFoo(T); This seems like a very common case, so it would be good to have a test explicitly for it.
  • a nested template, both where the inner template type is known, and where it is not.
brettw updated this revision to Diff 480536.Dec 6 2022, 10:12 AM
brettw marked an inline comment as done.
brettw marked an inline comment as done.

New patch up.

clang-tools-extra/test/clang-doc/single-file.cpp
19

I changed the existing test from relative line numbers which don't make sense given the structure of this (where the code is mostly static at the top, and the YAML is at the bottom which occasionally changes).

clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
631

I added the nested template example to the unit test. I also added a template parameter pack to the e2e test.

paulkirth accepted this revision.Dec 6 2022, 10:57 AM

This is basically LGTM. There are a few comments I think could be phrased a bit more clearly, but otherwise are more or less fine.

clang-tools-extra/clang-doc/Representation.h
120–124

Does this imply that its illegal to use "" as the name with this constructor? if so, it probably requires at least an assert.

201–203

I found this comment a bit hard to parse. Do you think this is a good way to rephrase?

211–213

I think this is equivalent. Do you mean this contains all instantiations of the parameter across the codebase? or for a single declaration?

225–226

Can you rephrase this? I think you're saying this type contains template information for a function or type that contains a template or template specialization. Is that correct?

This revision is now accepted and ready to land.Dec 6 2022, 10:57 AM
brettw updated this revision to Diff 480560.Dec 6 2022, 11:28 AM
brettw marked 5 inline comments as done.
brettw added inline comments.
clang-tools-extra/clang-doc/Representation.h
120–124

No, I meant something else, clarified.

211–213

Clarified

paulkirth added inline comments.Dec 6 2022, 11:32 AM
clang-tools-extra/clang-doc/Representation.h
120–124

Ah, I see now. Thank you.

206
This revision was automatically updated to reflect the committed changes.
brettw updated this revision to Diff 480979.Dec 7 2022, 10:52 AM

New patch with llvm::Optional converted to std::optional (previously collided with a refactor).

brettw reopened this revision.Dec 7 2022, 10:53 AM
This revision is now accepted and ready to land.Dec 7 2022, 10:53 AM
This revision was automatically updated to reflect the committed changes.