Adding -add-override functionality to cpp11-migrate tool

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.

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?

add-override-clang_tools_extra_174296.patch

Adds the -add-override refactor to the cpp11-migrate tool

  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?
  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.

Should I be using the git repo or llvm-reviews.chandlerc.com for these changes?

Thanks,
Philip Dunstan
phil@philipdunstan.com
http://philipdunstan.com/

add-override-clang_174296.patch (2.92 KB)

add-override-clang_tools_extra_174296.patch (16.5 KB)

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"

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

Philip,

You can find an example of that in r164892.

Dmitri

What we've done with testing so far is if the input file has C++11 features in it then the command line parameters you provide to the tool (either by the compile_commands.json file or after the -- on the command line) should include the -std=c++11 flag. Basically, however you compile the input code with clang normally should be identical to how you use the tool.

That said, the final syntax check does get run with -std=c++11 added.

Gribozavr wrote:

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.

Thanks Edwin, that makes sense.

Thanks Dmitri and Matthieu,

At the moment I am not doing anything like this. I am simply inserting the “override” keyword in AddOverrideActions. Should I be appling a similar technique to find an appropriate macro to apply instead of the raw override keyword?

Looking at how it is done for the fix-it hints for [[clang::fallthrough]] I appear to need to access a Preprocessor object. Is that still available at the time the AST matchers MatchCallback is run and what is the recommended way to access it?

I think inserting ‘override’ is a good place to start. If we want to support macros like LLVM_OVERRIDE maybe we could expose extra command-line arguments so the user can provide the name of ‘override’ (and possibly which header it belongs to in case the macro isn’t already defined).

The preprocessor object for a file is created by things ClangTool does and is kinda buried. I’m not sure what the best way to access that is. I do know that trying to re-pre-process a file results in an a thrown assertion. I think I had to create my own preprocessor to get around it.

Thanks everyone for your feedback,

I’ve attached a new set of patches with the changes suggested by Dmitri, Matthieu and Edwin. I implemented many of the suggested changes but there are a couple of changes that I made or didn’t make that I wanted to highlight.

I’ve replaced the use of the isCanonicalDecl matcher with hasParent(recordDecl()) as suggested by Dmitri. I have removed the addition of the isCanonicalDecl matcher from my patch to ASTMatchers.h and ASTMatchersTests.cpp.

I haven’t modified yet how the insertion point for the override keyword is calculated. Dmitri suggested FunctionTypeLoc::getRParenLoc() but I don’t think that this will work if there is a trailing const keyword or deferred return type. It’s possible that with a bit of work this could be made to work, but I am concerned that there might be other situations where this wouldn’t work that I am not aware of.

Following Edwin’s suggestion I am passing -std=c++11 to the cpp11-migrate tool in the unit tests to fix the warnings I was seeing.

add-override.clang_tools_extra.174386.patch (16.1 KB)

add-override-clang.174386.patch (2.31 KB)

Thanks everyone for your feedback,

I’ve attached a new set of patches with the changes suggested by Dmitri, Matthieu and Edwin. I implemented many of the suggested changes but there are a couple of changes that I made or didn’t make that I wanted to highlight.

I’ve replaced the use of the isCanonicalDecl matcher with hasParent(recordDecl()) as suggested by Dmitri. I have removed the addition of the isCanonicalDecl matcher from my patch to ASTMatchers.h and ASTMatchersTests.cpp.

I haven’t modified yet how the insertion point for the override keyword is calculated. Dmitri suggested FunctionTypeLoc::getRParenLoc() but I don’t think that this will work if there is a trailing const keyword or deferred return type. It’s possible that with a bit of work this could be made to work, but I am concerned that there might be other situations where this wouldn’t work that I am not aware of.

Following Edwin’s suggestion I am passing -std=c++11 to the cpp11-migrate tool in the unit tests to fix the warnings I was seeing.

I think this is definitely more ambitious, so something that can be tacked on later. As Vane said, at worst it could simply be an option: --add-override=LLVM_OVERRIDE.

There is certainly no reason to defer adding the new transformation for that.

– Matthieu

Could you please add a test with a trailing return type anyway?

class A {
public:
  virtual int h();
};

class B {
public:
  auto h() -> int;
};

Dmitri

I’ve attached patches that include this new test:

add-override.clang_tools_extra_174386.patch (16.3 KB)

add-override-clang.174386.patch (2.31 KB)

In the past I found that the semi-colon for lines doesn’t seem to be included in the source range for statements. I ended up using an instance of the lexer to locate it. You might be able to do something similar to find the right-most parenthesis. Alternatively, clang should probably just be improved to provide that sort of info.

Any news on this transform?

Sorry, I haven’t had a chance to chase this up lately. I’ve been changing jobs and then on holiday for a couple of weeks with limited internet access.

I posted the latest set of patches to cfe-commits on February 13 but I don’t think they were ever committed. Do I need to do something in particular to have the patches applied?

thanks.

I don’t see any emails from you to cfe-commits on Feb 13. Last email I see is from Feb 4 when the patches were still under review and you were working on some problems. I don’t remember ever seeing a reviewer’s approval. Could you send the patches again?

Sure. I will sync to latest and re-create the patches against a more recent revision.

PS. This was the email from Feb 13. http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130211/074110.html

Phil