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

[Dmitri Gribenko]

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>*/