[PATCH] D138073: [clang-doc] Move file layout to the generators.

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

Can you document the new directory structure with a small example? It would also be good to document in the code that some of these changes were driven by file corruption, and if possible point to a bug on github.



================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:890-892
+    // TODO If there are multiple Infos for this file name (for example,
+    // template specializations), this will generate multiple complete web pages
+    // (with <DOCTYPE> and <title>, etc.) concatenated together. This generator
----------------
So does this patch make HTML unusable? Currently, if there is a conflict, the file gets replaced, right? 

I read this as we're going from a situation where we lost data, but still had valid HTML pages to one where we have complete, but incorrect HTML, is that correct?

Also, why does that happen under template specialization? USRs are SHA1 hashes of the symbol. I would expect that hashing the symbol would be unambiguous and unique, given C++'s ODR. 

That last point made me wonder if an alternate way to handle this without using the hash would be to use the mangled names?


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:860
+
+  // Collect all output by file name and create the nexessary directories.
+  llvm::StringMap<std::vector<doc::Info *>> FileToInfos;
----------------
Nit: necessary 


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:893
+    // (with <DOCTYPE> and <title>, etc.) concatenated together. This generator
+    // needs some refactoring to be able to output the headers separatelky from
+    // the contents.
----------------
Nit: separately 


================
Comment at: clang-tools-extra/clang-doc/MDGenerator.cpp:382
+
+  // Collect all output by file name and create the nexessary directories.
+  llvm::StringMap<std::vector<doc::Info *>> FileToInfos;
----------------
nit: necessary


================
Comment at: clang-tools-extra/clang-doc/MDGenerator.cpp:422-435
+llvm::Error MDGenerator::createResources(ClangDocContext &CDCtx) {
+  // Write an all_files.md
+  auto Err = serializeIndex(CDCtx);
+  if (Err)
+    return Err;
+
+  // Generate the index page.
----------------
Why did this move? Nothing has changed in its implementation, right? 


================
Comment at: clang-tools-extra/test/clang-doc/single-file-public.cpp:6
 // RUN: clang-doc --doxygen --public --executor=standalone -p %t %t/test.cpp -output=%t/docs
-// RUN: cat %t/docs/GlobalNamespace/Record.yaml | FileCheck %s --check-prefix=CHECK
+// RUN: cat %t/docs/`ls %t/docs | grep -v index.yaml` | FileCheck %s --check-prefix=CHECK
 // RUN: rm -rf %t
----------------
Its OK to use `ls` and `grep` in the runline, but you cannot use the backtick syntax. It will cause problems on other platforms and I can't find any other usage of that in our codebase. TBH I'm a bit surprised this works, since `lit` has some interesting behavior around RUN. 

I think you can just use `cat < ls %t/docs | grep -v index.yaml` instead. I haven't tried it, but that is certainly a more typical method of doing this in a lit test. Using `find` may also work, but I only found one use in `llvm/test/ExecutionEngine/MCJIT/load-object-a.ll`.


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

https://reviews.llvm.org/D138073