This is an archive of the discontinued LLVM Phabricator instance.

[clang-doc] Fix link generation
ClosedPublic

Authored by DiegoAstiazaran on Jul 18 2019, 4:03 PM.

Details

Summary

Before making a link to a reference it is required to check that the reference has a path (eg. primitives won't have paths).
This was done by checking if the path was empty; that worked because when generating paths the outdirectory was included, so if the path was assigned it had that outdirectory at least.
The path generation was changed, it's now only the composite of the namespaces without the outdirectory. So if the info is in the global namespace the path would be empty and the old check wouldn't work as expected.
A new attribute has been added to the Reference struct that indicates if the info's parent is the global namespace.
Paths generation now fails if the path is empty and if the info is not in the global namespace.

Diff Detail

Event Timeline

Under what circumstances, exactly, is the path not set where you would need to create a link despite that? Is it only in the global namespace case? If so, could you special-case that and ignore other empty paths?

clang-tools-extra/clang-doc/Representation.h
142

Is this field actually set anywhere?

DiegoAstiazaran marked an inline comment as done.Jul 25 2019, 10:32 AM
DiegoAstiazaran added inline comments.
clang-tools-extra/clang-doc/Representation.h
142

Yes, in the constructors where a path is assigned.

Under what circumstances, exactly, is the path not set where you would need to create a link despite that? Is it only in the global namespace case? If so, could you special-case that and ignore other empty paths?

It's not specifically in the GlobalNamespace info file. It happens in that one but also in any info whose parent namespace is the global namespace.

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

Change attribute used to check special case of global namespace; IsPathValid changed to IsInGlobalNamespace. This attribute is true when its first parent is the global namespace or if the info is the global namespace,

jakehehrlich accepted this revision.Aug 2 2019, 12:23 PM

The code looks fine to me but I don't understand the global details of this. Hopefully Julie can still review things from there but if not we don't get timely reviews then we can still land this.

clang-tools-extra/clang-doc/Representation.h
140–141

Here and below perhaps favor putting the comments above the line rather than to the side to make the formatter produce nicer looking code.

clang-tools-extra/unittests/clang-doc/YAMLGeneratorTest.cpp
83–84

Maybe put the comment above this line so that the formatter doesn't get all wonky.

This revision is now accepted and ready to land.Aug 2 2019, 12:23 PM
DiegoAstiazaran marked 3 inline comments as done.

Fix comments

juliehockett accepted this revision.Aug 5 2019, 4:57 PM

LGTM with a comment correction

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

s/globalnamespace/global namespace

123

s/globalnamespace/global namespace

DiegoAstiazaran marked 2 inline comments as done.

Fix comments.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2019, 5:10 PM