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

[hulxv]

Fix #131697

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: Mohamed Emad (hulxv)

Changes

Fix #131697

---

Full diff: https://github.com/llvm/llvm-project/pull/131698.diff

1 Files Affected:

• (modified) clang-tools-extra/clang-doc/HTMLGenerator.cpp (+1-1)

diff --git a/clang-tools-extra/clang-doc/HTMLGenerator.cpp b/clang-tools-extra/clang-doc/HTMLGenerator.cpp
index 18a0de826630c..37d94ace17477 100644
--- a/clang-tools-extra/clang-doc/HTMLGenerator.cpp
+++ b/clang-tools-extra/clang-doc/HTMLGenerator.cpp
@@ -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)
     return std::make_unique<TagNode>(
         HTMLTag::TAG_P, "Defined at line " + std::to_string(L.LineNumber) +
                             " of file " + L.Filename);

[ilovepi]

Thanks for the PR, but I don't think this is logic is quite right as it stands. There should also be some accompanying test changes to reflect what's being fixed. As this patch stands it removes basic functionality.

[ilovepi]

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.

[ilovepi]

#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?

[hulxv]

#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

[hulxv]

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

[ilovepi]

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.

[hulxv]

@ilovepi
I see you opened a PR to solve the same issue here. Wouldn't be better if I continue working on it with your suggestions? 😅

[ilovepi]

@ilovepi I see you opened a PR to solve the same issue here. Wouldn't be better if I continue working on it with your suggestions? 😅

I'm fine either way, but I wanted some kind of testing so I could verify the fix.

[ilovepi]

hey, sorry, I ended up landing #131939. There are two reasons: 1) I really wanted to fix that crash ASAP. 2) I made the mistake of stacking other patches on top of it with graphite, and it turns out it isn't easy to reorder them. As a result I couldn't fix the basic functionality of HTML links without reuploading the entire rest of the stack. Normally, I'd much prefer to just mentor you through the process, but I really needed to fix those links.

If you're still interested in working on this though, I think the test could be improved , and I wouldn't mid seeing that code cleaned up. In particular, I'm wondering how great of an idea it is to use those optionals, when we end up w/ a .value_or(""). The amount of unique_ptr we're using is also not the greatest. And I wouldn't be surprised if using ASan/TSan on something like LLVM wouldn't find several bugs. Lastly, the testing story in clang-doc is quite weak. We've improved it a lot over last year's GSOC, but I still don't think its all that well exercised.