[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:
> > 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.


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:9-10
+//
+// clang plugin, checks every overridden virtual function whether called
+// this function or not.
+//
----------------
How about: `This Clang plugin checks that overrides of the marked virtual function directly call the marked function.`


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:80
+
+      // Now find if supper is used in `MethodDecl`.
+      MethodUsageVisitor Visitor(OverriddenMarkedMethods);
----------------
supper is used -> the superclass method is called


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:84
+      // After traversing, all methods left in `OverriddenMarkedMethods`
+      // are not get called, warning about these.
+      for (const auto &LeftOverriddens : OverriddenMarkedMethods) {
----------------
are not get called, warning -> are not called, warn


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:152
+      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type_str)
+          << Attr << "methods";
+      return false;
----------------
Instead of `methods`, how about `virtual functions`? Then the logic can be: `if (!TheMethod || !TheMethod->isVirtual())` and you can remove the diagnostic below.


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:166-168
+    // No need to add an attr object (usually an annotation attr is added)
+    // save the address of the Decl in sets, it maybe faster than compare to
+    // strings.
----------------
attr is added) save the address of the Decl in sets -> .attr is added). Save the address of the Decl in a set


================
Comment at: clang/test/Frontend/plugin-call-super.cpp:1-2
+// RUN: %clang -fplugin=%llvmshlibdir/CallSuperAttr%pluginext -emit-llvm -S %s -o - 2>&1 | FileCheck %s --check-prefix=CALLSUPER
+// RUN: not %clang -fplugin=%llvmshlibdir/CallSuperAttr%pluginext -emit-llvm -DBAD_CALLSUPER -S %s -o - 2>&1 | FileCheck %s --check-prefix=BADCALLSUPER
+// REQUIRES: plugins, examples
----------------
The way we would typically test something like this is to pass `-fsyntax-only` and `-verify` to verify the diagnostics rather than using FileCheck, because the plugin doesn't have any impact on code generation (just diagnostics). This makes the test faster to check (and is a bit easier than using FileCheck).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91047