brettw added inline comments.
================ 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 ---------------- paulkirth wrote: > 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? I think this makes HTML strictly better (though not ideal). Previously if there was a collision, they got written over the top of each other without truncation (not sure why). So if the longer page was written second, you got lucky and you get a complete valid page and just lost the shorter struct's definition. If you got unlucky the shorter page was written second and you get a corrupted file with the shorter content, followed by the end of the longer content peeking out from the end. With this change, you get both content concatenated without corruption, you just have duplicate doctype and title tags in between them. Browsers should generally handle this so the page should be usable. Fixing this requires a refactor of the HTML generator such that the concept of writing a struct is separate from writing a page. Even if I was going to work on the HTML generator, I would probably want to do this in a separate patch anyway. The markdown generator does the same, but markdown doesn't have all of the headers stuff that shouldn't be in the middle of the file, you just get a new `# ...` for the second section and it should be reasonable. The HTML and MD generators name files according to the `Name` of the structure. USR is not involved. So any time to things in the same namespace have the same name, you'll get a collision. This happens most commonly with template specialization because the names are identical (the name doesn't account for the template parameters). All of this complexity is why I changed the YAML output to use USR which avoids the problem. I didn't want to make this change for the HTML and MD generators because it will make ugly file names that the user will see, and you probably want the template specializations to be in the same file anyway. ================ 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 ---------------- paulkirth wrote: > 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`. Your example doesn't work (it's trying to load a file named "ls") but I found another solution using xargs (which is hopefully supported on all of the platforms). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138073/new/ https://reviews.llvm.org/D138073 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
