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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I have more review to do on this but have to run for a while and wanted to get you this feedback sooner rather than later.



================
Comment at: clang/docs/ClangPlugins.rst:119
+
+Attribute plugin to mark a virtual method as `call_super`, subclasses must call it
+in the overridden method.
----------------
Sorry, I was unclear, it should be *double* backticks around syntax elements like `call_super`, not single backticks.


================
Comment at: clang/docs/ClangPlugins.rst:122-123
+
+This example shows the possibility that attribute plugins combine with PluginASTAction
+in Clang can do same things which Java Annotations do.
+
----------------
Slight tweaking for grammar:

`This example shows that attribute plugins combined with PluginASTAction in Clang can do some of the same things which Java Annotations do.`

(with double backticks around `PluginASTAction`)


================
Comment at: clang/docs/ClangPlugins.rst:125
+
+Not like other attribute plugin examples, this one do not attached any attribute nodes
+to a declaration node, instead, saving the declaration addresses directly, which may
----------------
Slight tweaking for grammar:

```
Unlike the other attribute plugin examples, this one does not attach an attribute AST node to the declaration AST node. Instead, it keeps a separate list of attributed declarations, which may be faster than using Decl::getAttr<T>() in some cases. The disadvantage of this approach is that the attribute is not part of the AST, which means that dumping the AST will lose the attribute information, pretty printing the AST won't write the attribute back out to source, and AST matchers will not be able to match against the attribute on the declaration.
```
(with double backticks around `Decl::getAttr<T>()`)


================
Comment at: clang/docs/ClangPlugins.rst:117
+Defining CallSuperAttr
+===================
+
----------------
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.


================
Comment at: clang/docs/ClangPlugins.rst:119
+
+Attribute plugin to mark a virtual method as call_super, sub-classes must call it in overridden the method.
+
----------------
psionic12 wrote:
> psionic12 wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > Should add backticks around `call_super` since it's syntax. Also, `sub-classes` should be `subclasses`.
> > > > 
> > > > "call it in overridden the method" -> "call it in the overridden method"
> > > There should be more explanation here about what concepts the example demonstrates. For example, one interesting bit to me is that it shows how you can add a custom attribute that's useful even if it does not generate an AST node for the attribute.
> > "how you can add a custom attribute that's useful even if it does not generate an AST node for the attribute", do you mean I should add an Annotation Attribute object even I don't use it? So that when someone dumps the AST, the `call_super` attribute will show?
> > 
> > Or do you mean to explain the inner implementation of how could the RecursiveASTVisitor knows the declaration is marked as `call_super`, even if I didn't add any extra attribute nodes to this declaration node?
> I got you point, please ignore the comment above.
> 
> Since this is an example, I should mention more about this example itself rather than how to use this plugin, right?
> Since this is an example, I should mention more about this example itself rather than how to use this plugin, right?

Exactly!


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:63
+        DiagnosticsEngine::Warning,
+        "%0 is marked as call_super but override method %1 does not call it");
+    NotePreviousCallSuperDeclaration = Diags.getCustomDiagID(
----------------
psionic12 wrote:
> aaron.ballman wrote:
> > Should put single quotes around `call_super` as it's a syntactic element.
> > 
> > I'm not certain that %0 and %1 are necessary because the overridden methods will have the same names. I think you can drop the %1 and rely on the note to point out where the overridden method lives.
> > 
> About the '%0' and '%1' problem, same as the comment about "fullName()" function, since the overridden methods have the same name, I want use "class_name::method_name" to distinguish between these methods to make users clear.
> 
> It would very nice if you could help to come up with a decent warning message which is better than this, I tried some but none of them give a compiler-warning-style feeling... 
> It would very nice if you could help to come up with a decent warning message which is better than this, I tried some but none of them give a compiler-warning-style feeling...

How about:

`virtual function %q0 is marked as 'call_super' but this overriding method does not call the base version`

and

`"function marked 'call_super' here"`


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:90
+            Diags.Report(MethodDecl->getLocation(), WarningSuperNotCalled)
+                << fullName(Overridden) << fullName(MethodDecl);
+            Diags.Report(Overridden->getLocation(),
----------------
psionic12 wrote:
> aaron.ballman wrote:
> > FWIW, you can pass `Overridden` and `MethodDecl` in directly rather than trying to scrape the name out yourself -- the diagnostics engine knows how to properly turn a `NamedDecl` into a string for diagnostics.
> Same problem, a decent warning message is welcome, I don't like this scrape-name-thing either. 
You can use `%q0` to specify that you want a qualified name printed, I believe, which should do what you want with the function name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91047