[cfe-dev] Adding -add-override functionality to cpp11-migrate tool

[Matthieu Monrocq]

On Mon, Feb 4, 2013 at 1:13 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Mon, Feb 4, 2013 at 3:32 AM, Philip Dunstan <phil at philipdunstan.com>
> wrote:
> > Hi
> >
> > These patches continue some work that I started back in July (before RL
> > kicked in) to create a C++11 migration tool that would add the override
> > specifier to suitable member functions where it could. I have reworked
> the
> > tool to integrate it into the cpp11-migrate tool in the tool/extra
> > repository.
>
> Hi Philip,
>
> This looks like a great addition to the C++11 conversion tool!
>
> > There are two patches - one on the clang repo with some AST matchers and
> the
> > other in tool/extra. I have a couple of questions for each of the patches
> > that I would appreciate some help with.
> >
> >
> > add-override-clang_174296.patch
> > Adds ASTMatchers isCanonicalDecl, isVirtual and isOverride
> >
> > Questions:
> > 1. How do I unit test the negative of isCanonicalDecl?
> > 2. Will someone help me with a better description of isCanonicalDecl for
> the
> > comment?
>
> I'm unsure if isCanonicalDecl is what you want.
>
> methodDecl(hasParent(recordDecl())).bind("id") works the same way.
>
> > 1. I don't like the work in AddOverrideActions.cpp to identify the
> desired
> > location of the override keyword if there is an inlined body (work
> backwards
> > from the start of the body to the first non-whitespace character). This
> > doesn't work if there is a comment between the declaration and the body
> > (shown in the comment_before_inline_body_fail test) and I don't think it
> is
> > correct if there are user . Will someone please suggest a better way to
> do
> > this?
>
> This might require improvements in Clang source code.  But there's
> already a FunctionTypeLoc::getRParenLoc(), which might be what you
> want.
>
> > 2. I'm seeing a warning from the cpp11-migrate tool because C++11 is not
> > enabled in the compiler options during the initial parsing of the
> source. I
> > think that C++11 should be enabled as I expect this tool to be used on
> code
> > that has already been partly migrated to C++11. I would also expect that
> > this issue will arise as people run the migration tool with multiple
> > refactoring tool options.
>
> I agree.  Input file should compile as C++11 cleanly.
>
> > Should I be using the git repo or llvm-reviews.chandlerc.com for these
> > changes?
>
> You can do so, but you are not required to.
>
> +#include "llvm/Support/Compiler.h" // For LLVM_OVERRIDE
>
> In general, we don't write such comments.
>
> +  AddOverrideFixer(clang::tooling::Replacements &Replace,
> +               unsigned &AcceptedChanges,
> +               RiskLevel) :
>
> Please align 'clang' and 'unsigned'.
>
> If RiskLevel is not used now, why not drop it?
>
> +const char *Method = "method";
>
> I'm a bit worried about adding such definitions at TU scope.  I know
> there are such definitions in other transformations already...
>
> +  return methodDecl(isCanonicalDecl(),
> +                    isOverride(),
> +                    unless(destructorDecl()),
> +                    unless(hasOverrideAttr())
> +                   ).bind(Method);
>
> ').bind' looks weird.  Please move to the previous line.
>
> +// Names to bind with matched expressions.
> +extern const char *Method;
>
> Three slashes?
>
> +int AddOverrideTransform::apply(const FileContentsByPath &InputStates,
> +                               RiskLevel MaxRisk,
>
> Please align 'const' and 'RiskLevel'.
>
> +#include "clang/Lex/Lexer.h"
> +
> +
> +using namespace clang::ast_matchers;
>
> In general, we don't leave two consecutive empty lines.  (There are
> multiple occurrences of this.)
>
> +void AddOverrideFixer::run(const
> ast_matchers::MatchFinder::MatchResult &Result) {
>
> 80 cols.  You can drop 'ast_matchers::'.
>
> +  const CXXMethodDecl* M = Result.Nodes.getDeclAs<CXXMethodDecl>(Method);
>
> Space should be adjacent to 'M'.
>
> +      // start at the beginning of the body and rewind back to the last
>
> s/start/Start/
>
> +    }
> +    else {
>
> This should be '} else {'.
>
> +AST_MATCHER(clang::CXXMethodDecl, isOverride) {
> +  return (Node.size_overridden_methods() > 0);
> +}
>
> I don't know if this is generally useful.  You don't have to express
> everything as a matcher -- can filter out declarations in the
> callback.
>
> Dmitri
>
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>

Hi Philip,

I seem to remember there was some magic in the [[clang::fallthrough]]
attribute that allowed one to find the last macro that evaluated to this
attribute (should one exist), and thus allowed the fix-it hints to use the
macro spelling instead of the "pure" spelling.

Does your tool perform the same search ? Or do you just use the override
keyword ?

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130204/8e43fba5/attachment.html>