[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
+===================
+
----------------
The number of underlines here looks off -- can you verify it's correct?


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


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


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:25
+namespace {
+// cached methods which are marked as call_super
+llvm::SmallPtrSet<const CXXMethodDecl *, 16> ForcingCalledMethods;
----------------
Comments should be grammatical including starting with a capital letter and trailing punctuation. (Should be corrected elsewhere in the patch as well.)


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:32
+
+std::string fullName(const CXXMethodDecl *D) {
+  std::string FullName;
----------------
Is the functionality for getting names out of a `NamedDecl` insufficient?


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:43
+  bool IsOverriddenUsed = false;
+  MethodUsageVisitor(const clang::CXXMethodDecl *MethodDecl)
+      : MethodDecl(MethodDecl) {}
----------------
ctor should probably be `explicit`


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:43
+  bool IsOverriddenUsed = false;
+  MethodUsageVisitor(const clang::CXXMethodDecl *MethodDecl)
+      : MethodDecl(MethodDecl) {}
----------------
aaron.ballman wrote:
> ctor should probably be `explicit`
Can drop `clang::` prefixed everywhere since you have a `using` declaration at the top of the file.


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



================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:66
+        DiagnosticsEngine::Note,
+        "overridden method which is marked as call_super here");
+  }
----------------
Single quotes around `call_super` here as well; we usually phrase this sort of thing as `"overridden method marked 'call_super' here`


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:73
+          DiagnosticsEngine::Warning,
+          "call_super attribute attached on a final method");
+      Diags.Report(MethodDecl->getLocation(), ID);
----------------
Single quotes around `call_super` and we should use consistent terminology for "attached" vs "marked".


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:76
+    }
+    for (const auto *Overridden : MethodDecl->overridden_methods()) {
+      if (isMarkedAsCallSuper(Overridden)) {
----------------
Won't this visit every method declaration multiple times? Once for the declaration itself (as we encounter it) and once for each time it's listed as an overridden method?


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:80
+        // skip checking
+        if (!MethodDecl->isThisDeclarationADefinition()) {
+          continue;
----------------
We typically elide the braces for single-line substatements.


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


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:135
+    static constexpr Spelling S[] = {{ParsedAttr::AS_GNU, "call_super"},
+                                     {ParsedAttr::AS_CXX11, "call_super"}};
+    Spellings = S;
----------------
For the C++11 spelling, it should be in the `clang` attribute namespace rather than the standard attribute namespace.


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:150
+          DiagnosticsEngine::Error,
+          "call_super attribute must attached on a virtual function");
+      S.Diags.Report(D->getLocation(), ID);
----------------
Single quotes around the attribute name.


================
Comment at: clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp:171
+    Y("call_super_attr", "Attr plugin to define call_super attribute");
\ No newline at end of file

----------------
Can you add a newline to the end of the file?


================
Comment at: clang/test/Frontend/plugin-call-super.cpp:7
+struct Base2 { [[call_super]] virtual void Test() {} };
+struct Derive : public Base1, public Base2 { [[call_super]] virtual void Test() override final; };
+// CALLSUPER: warning: call_super attribute attached on a final method
----------------
psionic12 wrote:
> If I remove the keyword "virtual" here, the isVirtual() will return false.
> 
> To my knowledge "final" means this method cannot be overridden, but it is still a virtual method, right?
Only virtual functions can be marked `final`, so yes, it's still a virtual function. `isVirtual()` tells you whether the user wrote `virtual` themselves, but I'm not certain why `isVirtual()` would return false there -- the function overrides a method, so I would have expected this to return true: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/DeclCXX.h#L2023


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91047