[PATCH] D91047: Add a call super attribute plugin example

[Yafei Liu via Phabricator via cfe-commits]

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