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