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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
