Skip to content

[clang-doc][fix] crashes when generating HTML without --repository #131698

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[clang-doc][fix] crashes when generating HTML without --repository #131698

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang-tools-extra/clang-doc/HTMLGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ genReferencesBlock(const std::vector<Reference> &References,
static std::unique_ptr<TagNode>
writeFileDefinition(const Location &L,
std::optional<StringRef> RepositoryUrl = std::nullopt) {
if (!L.IsFileInRootDir && !RepositoryUrl)
if (!L.IsFileInRootDir || !RepositoryUrl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic isn't correct, and happens to prevent us from effectively using the --repository= string anywhere, as evidenced by our tests.

I have some additional test changes that I'm in the process of adding that will hopefully cover this case.

An alternative would be to change the deref of the option to use .value_or("") instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#131894 adds more complete testing logic around the --repository flag. see if the behavior you're trying to fix reproduces in the tests w/ that PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#131894 adds more complete testing logic around the --repository flag. see if the behavior you're trying to fix reproduces in the tests w/ that PR?

yup, it's correct but I think we need to add some tests for single files also

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic isn't correct, and happens to prevent us from effectively using the --repository= string anywhere, as evidenced by our tests.

The whole next logic depends on RepositoryUrl. how can we continue using it if it's not passed?

An alternative would be to change the deref of the option to use .value_or("") instead.

If RepositoryUrl is not passed, shouldn't we use file:// protocol as I think? In this case, the FileUrl should be something like file:///path/to/file, not just an empty string

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole next logic depends on RepositoryUrl. how can we continue using it if it's not passed?

What I mean is that often !L.IsFileInRootDir || !RepositoryUrl is true when we want to use the --repository flag, meaning we never use the URL in situations where we should.

If RepositoryUrl is not passed, shouldn't we use file:// protocol as I think? In this case, the FileUrl should be something like file:///path/to/file, not just an empty string

Maybe. I think we're still generating a valid path in the HTML, but different generation methods are welcome. Some of this may be obsoleted after @PeterChou1 finishes landing #108653 and the prerequisite Mustache support in LLVM support.

return std::make_unique<TagNode>(
HTMLTag::TAG_P, "Defined at line " + std::to_string(L.LineNumber) +
" of file " + L.Filename);
Expand Down
Loading