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

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:376
+  LocNumberNode->Attributes.try_emplace(
+      "href", (FileURL + "#" + std::to_string(L.LineNumber)).str());
+  Node->Children.emplace_back(std::move(LocNumberNode));
----------------
Add a comment here that this is the github/googlesource notation for this that way in the future people arren't confused when it doesn't work for other hosting pages


================
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>(
----------------
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.


================
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, "");
----------------
Do you actually need to do this? Feels like a bug in replace_path_prefix per its own documentation if you do.


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:228
+      RepositoryUrl.find("https://") != 0)
+    RepositoryUrl.insert(0, "http://");
+
----------------
1) Do we need to add this for the user?
2) Can we use https by default if we need this?


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

https://reviews.llvm.org/D65483