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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/ClangPlugins.rst:117
+Defining CallSuperAttr
+===================
+
----------------
psionic12 wrote:
> 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.
You can test in your own environment (if a build bot can do it, so can anyone else). I don't typically build docs locally so my instructions are untested, but I think you need to set the following cmake variables:

```
LLVM_BUILD_DOCS=YES ' enables building docs
LLVM_ENABLE_SPHINX=YES ' enable building sphinx docs (.rst files)
```
You also have to have Sphinx installed (you can do this using `pip install -U Sphinx`) and on the PATH before you configure cmake for Clang.

Doxygen is a different documentation format than Sphinx. Doxygen handles building the docs from comments in the source files. Sphinx is what turns these .rst files into HTML. If you enable Sphinx builds from cmake there should be a target added that lets you build those docs.


================
Comment at: clang/test/Frontend/plugin-call-super.cpp:5
+
+// expected-no-diagnostics
+struct Base1 { [[clang::call_super]] virtual void Test() {} };
----------------
Hmm, this seems wrong to me -- we do expect diagnostics from this file.


================
Comment at: clang/test/Frontend/plugin-call-super.cpp:18-19
+struct Derive2 : public Base1, public Base2 { void Test() override {  Base1::Test();  Base2::Test();}};
+// BADCALLSUPER: warning: virtual function 'Base2::Test' is marked as 'call_super' but this overriding method does not call the base version
+// BADCALLSUPER: note: function marked 'call_super' here
+#endif
----------------
These warnings and notes (and the warning a few lines up) are ones I would have expected to catch using `// expected-warning {{virtual function 'Base2::Test' is marked as 'call_super' but this overriding method does not call the base version}}` style checks instead of needing to use FileCheck.

Do plugin-based diagnostics not get caught by `-verify`? I expect this test file to fail as currently written because of the `expected-no-diagnostics`, but I've not done a whole lot of testing of plugins before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91047