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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to