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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett 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>(
----------------
DiegoAstiazaran wrote:
> 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?
While the tooltip idea seems interesting, I'm inclined to keep the "Defined at" portion and an actual link with the text for now. I'd want to see an example of what the tooltip option would look like before going with it. 

For now, the separate line number and file linkification SGTM.


================
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, "");
----------------
DiegoAstiazaran wrote:
> 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.
Leave a comment to that effect, then.


================
Comment at: clang-tools-extra/clang-doc/Mapper.cpp:40
+  bool IsFileInRootDir;
+  auto File = getFile(D, D->getASTContext(), CDCtx.RootDir, IsFileInRootDir);
+  auto I = serialize::emitInfo(D, getComment(D, D->getASTContext()),
----------------
Don't use auto here, since it's not immediately obvious what the type is.


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:228
+      RepositoryUrl.find("https://") != 0)
+    RepositoryUrl.insert(0, "http://");
+
----------------
DiegoAstiazaran wrote:
> 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.
This is fine, but please add a test case for the when the user omits the prefix.


================
Comment at: clang-tools-extra/clang-doc/tool/ClangDocMain.cpp:76
+static llvm::cl::opt<std::string>
+    RootDirectory("root-directory", llvm::cl::desc(R"(
+Directory where processed files are stored.
----------------
Can we call this `--source-root`?


================
Comment at: clang-tools-extra/docs/clang-doc.rst:93
+    --public                    - Document only public declarations.
+    --repository=<string>       -
+                                  URL of repository that hosts code.
----------------
Formatting here is a bit weird


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

https://reviews.llvm.org/D65483