Page MenuHomePhabricator

[clang-doc] Add html links to references
ClosedPublic

Authored by DiegoAstiazaran on Fri, Jun 21, 12:21 PM.

Details

Summary

<a> tags are added for the parents and members of records and return type and params of functions.
The link redirects to the reference's info file.
The directory path where each info file will be saved is now generated in the serialization phase and stored as an attribute in each Info.
Bitcode writer and reader were modified to handle the new attributes.

Diff Detail

Repository
rL LLVM

Event Timeline

General design thought: could we move the path name construction to Serialize.cpp? Essentially, move the logic that builds the path name from the namespaces and name to the serialize step (you'll have to add the OutDirectory to the ClangDocContext so that you can access it in the mapper), and then the GetInfoOuputFile() function in ClangDocMain.cpp would take the Info.Path string as a parameter and do the directory construction and whatnot.

With that, you could then also add the Path field to References, so that you wouldn't need to pass a map around in the generation phase. What do you think?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-doc/HTMLGenerator.cpp
21 ↗(On Diff #206049)

Anonymous namespace is used for functions when LLVM Coding Guidelines tells using static for same purpose.

31 ↗(On Diff #206049)

You don't need to assign empty string. See readability-redundant-string-init.

DiegoAstiazaran edited the summary of this revision. (Show Details)

Path name generation was moved to serialization phase as @juliehockett suggested.
Empty string declaration fixed.
Removed anonymous namespaces.

DiegoAstiazaran marked 2 inline comments as done.Mon, Jun 24, 4:01 PM
ormris removed a subscriber: ormris.Mon, Jun 24, 4:04 PM
juliehockett added inline comments.Mon, Jun 24, 5:00 PM
clang-tools-extra/clang-doc/HTMLGenerator.cpp
58 ↗(On Diff #206317)

Use llvm::sys::path to allow for other platforms that use a different separator.

21 ↗(On Diff #206049)

Actually, since these aren't used outside of this file, please use the static keyword instead of wrapping them in a namespace.

clang-tools-extra/clang-doc/MDGenerator.cpp
21 ↗(On Diff #206317)

Same here, just use static.

clang-tools-extra/clang-doc/Serialize.cpp
31 ↗(On Diff #206317)

<root>/A/B/C.<ext>

230 ↗(On Diff #206317)

Also for members

255 ↗(On Diff #206317)

Also for param references, if applicable

308–310 ↗(On Diff #206317)

Can we do the path construction for virtual bases as well, so they can be linked too?

341–350 ↗(On Diff #206317)

Add path information to these references as well

clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
145–146 ↗(On Diff #206317)

formatting change?

DiegoAstiazaran marked 10 inline comments as done.
DiegoAstiazaran retitled this revision from [clang-doc] Add html links to the parents of a RecordInfo to [clang-doc] Add html links to references.
DiegoAstiazaran edited the summary of this revision. (Show Details)

Links added for a function's return type and parameters' types and for a record's members' type.
Fixed path generation to support multiple platforms.

clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
145–146 ↗(On Diff #206317)

Yes, clang-format did that change.

DiegoAstiazaran edited the summary of this revision. (Show Details)

The links are now generated using the nodes structure introduced by D63857.

Rebase after D52847 was pushed.
Path added to parent Info in serialization, this is required after D63911 was pushed.

The code looks good to me. I'll let Julie give the final architectural approval.

juliehockett added inline comments.Tue, Jul 9, 9:24 AM
clang-tools-extra/clang-doc/HTMLGenerator.cpp
206 ↗(On Diff #208463)

You shouldn't need the prefixed ::

clang-tools-extra/clang-doc/Serialize.cpp
466 ↗(On Diff #208463)

While you're here, can you update this error to something like "Invalid reference type for parent namespace"?

567 ↗(On Diff #208463)

nit: use ++Enum.Namespace.begin()

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
219 ↗(On Diff #208463)

Could it add the OutDirectory here, and just store the relative path in I->Path? Since OutDirectory is constant throughout. You also wouldn't have to plumb it through all the serialize functions.

DiegoAstiazaran marked 5 inline comments as done.

The info path generated in serialization was the composite of the out directory and the parent namespaces. Now it's only the parent namespaces; the out directory path is appended until the directory is actually created in the doc generation phase.

DiegoAstiazaran added inline comments.Tue, Jul 9, 1:15 PM
clang-tools-extra/clang-doc/Serialize.cpp
567 ↗(On Diff #208463)

++ operator cannot be used in that expression because it's not assignable.

I've also just noticed that the YAML generator is missing from this -- could you add the Path fields to hat output as well?

clang-tools-extra/clang-doc/HTMLGenerator.cpp
213 ↗(On Diff #208789)

Now that the paths aren't absolute, you'll need to make this relative to the current path (something like https://github.com/llvm/llvm-project/blob/master/clang/lib/Lex/PPLexerChange.cpp#L201)

Use relative paths for references/links within files.
Don't fill namespaces of parentInfo in serialization and path is only generated when the parent is the global namespace.
Add new fields (Path in Reference and Path in Info) to YAML output and tests.

juliehockett added inline comments.Wed, Jul 10, 3:58 PM
clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
110–113 ↗(On Diff #209053)

This text should be rendered inline

168–171 ↗(On Diff #209053)

This text should be rendered inline, if possible.

DiegoAstiazaran marked an inline comment as done.Wed, Jul 10, 4:01 PM

In tag nodes with children that are not inline, two text nodes that are adjacent won't have a new line between them. Tag nodes are still rendered in their own line.

<p>
  A
   B
  <a>C<a>
  D
</p>

is now rendered as

<p>
  A B
  <a>C<a>
  D
</p>
DiegoAstiazaran marked 2 inline comments as done.Wed, Jul 10, 4:54 PM
juliehockett accepted this revision.Thu, Jul 11, 8:38 AM
This revision is now accepted and ready to land.Thu, Jul 11, 8:38 AM
DiegoAstiazaran edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jul 12, 11:31 AM