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

[Yafei Liu via Phabricator via cfe-commits]

psionic12 added inline comments.


================
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.
+
----------------
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?


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:32
+
+std::string fullName(const CXXMethodDecl *D) {
+  std::string FullName;
----------------
aaron.ballman wrote:
> Is the functionality for getting names out of a `NamedDecl` insufficient?
`fullName()` here is used to get a string which format is "class_name::method_name", I think NamedDecl::getName or NamedDecl::getNameAsString only gets the method name, without the class name.

Or is there an easy way to accomplish this?


================
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(
----------------
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... 


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:90
+            Diags.Report(MethodDecl->getLocation(), WarningSuperNotCalled)
+                << fullName(Overridden) << fullName(MethodDecl);
+            Diags.Report(Overridden->getLocation(),
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91047