[PATCH] D65483: [clang-doc] Add link to source code in file definitions

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran added a subscriber: phosek.
DiegoAstiazaran added inline comments.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:377-378
+      "href", (FileURL + "#" + std::to_string(L.LineNumber)).str());
+  Node->Children.emplace_back(std::move(LocNumberNode));
+  Node->Children.emplace_back(llvm::make_unique<TextNode>(" of file "));
+  auto LocFileNode = llvm::make_unique<TagNode>(
----------------
jakehehrlich wrote:
> Can you put these in the link so that the link is larger than a single number? e.g. "303 of file foo.c" should be linkified rather than just "303" which is how I read the code now.
"303" is linkified to the specific line in the source code but also "foo.c" is linkified to the top of the source code page.
This is how it is done in Doxygen documentation.
I think it would look weird to have " of file " linkified. But talking to @phosek, we agreed that it would be better to remove the whole "Defined at ..." and make the info name linkified to the definition, with a tooltip that shows "foo.c:303".
So I think we could leave it like this for now and later, in another patch (this will require some CSS), do what I just mentioned. What do you think?


================
Comment at: clang-tools-extra/clang-doc/Mapper.cpp:103-104
+  llvm::SmallString<128> Prefix(RootDir);
+  if (!llvm::sys::path::is_separator(Prefix.back()))
+    Prefix += llvm::sys::path::get_separator();
+  llvm::sys::path::replace_path_prefix(File, Prefix, "");
----------------
jakehehrlich wrote:
> Do you actually need to do this? Feels like a bug in replace_path_prefix per its own documentation if you do.
`replace_path_prefix` simply calls substr() on the path so if the prefix does not include the separator at the end, the resulting path will have it at the beginning.


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:228
+      RepositoryUrl.find("https://") != 0)
+    RepositoryUrl.insert(0, "http://");
+
----------------
jakehehrlich wrote:
> 1) Do we need to add this for the user?
> 2) Can we use https by default if we need this?
Not required but I consider it'd be nice to have it.
You're right, https should be the default.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65483/new/

https://reviews.llvm.org/D65483