psionic12 marked 6 inline comments as done. psionic12 added inline comments.
================ Comment at: clang/docs/ClangPlugins.rst:117 +Defining CallSuperAttr +=================== + ---------------- aaron.ballman wrote: > psionic12 wrote: > > aaron.ballman wrote: > > > aaron.ballman wrote: > > > > The number of underlines here looks off -- can you verify it's correct? > > > This still appears to be incorrect and will cause build errors for the > > > documentation. > > Do you mean that there's a command to build the documentation and currently > > my patch will cause a failure on it? > > > > I thought this ClangPlugins.rst is only an documentation with markdown, but > > seems that it's not what I thought? > > > > Currently I will make sure there's no build error on the plugin itself and > > the regression test case, and make sure the > > regression test will pass. Seems that's not enough, right? > > Do you mean that there's a command to build the documentation and currently > > my patch will cause a failure on it? > > Yes, we have a bot that builds docs: http://lab.llvm.org:8011/#/builders/92 > > > I thought this ClangPlugins.rst is only an documentation with markdown, but > > seems that it's not what I thought? > > It is a documentation file with markdown, but the markdown bots will complain > if a markdown file cannot be built (they treat markdown warnings as errors). > > > Currently I will make sure there's no build error on the plugin itself and > > the regression test case, and make sure the regression test will pass. > > Seems that's not enough, right? > > Most of the time that's enough because the markdown usage is pretty trivial > and can be inspected by sight for obvious issues (so people don't typically > have to set up their build environments to generate documentation and test it > with every patch). > > In this case, the issue is with the underlines under the title. The number of > underlines needs to match the number of characters in the title, but in this > case there are 20 `=` characters but 23 title characters. It seems that only a committee member can trigger this bot, any way I can test on my own environment? So that I can make sure the doc will compile successfully before uploading patches? As I mentioned before, using `make doxygen-clang` will succeed even the `=` characters are not match with title characters, so it seems that the bot doesn't use this way. ================ Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:104 + for (const auto *Method : MarkedMethods) { + lateDiagAppertainsToDecl(Diags, Method); + } ---------------- Move the `lateDiagAppertainsToDecl()` here and only check marked functions. ================ Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:69 + bool VisitCXXMethodDecl(CXXMethodDecl *MethodDecl) { + lateDiagAppertainsToDecl(MethodDecl); + ---------------- Call `lateDiagAppertainsToDecl` in every `VisitCXXMethodDecl` functions is not very efficiency, since marked methods are already saved. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91047/new/ https://reviews.llvm.org/D91047 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
