Page MenuHomePhabricator

[clang-tidy] add OverrideMacro to modernize-use-override check
ClosedPublic

Authored by MyDeveloperDay on Jan 23 2019, 1:20 AM.

Details

Summary

The usefulness of modernize-use-override can be reduced if you have to live in an environment where you support multiple compilers, some of which sadly are not yet fully C++11 compliant

some codebases have to use override as a macro OVERRIDE e.g.

#if defined(COMPILER_MSVC)
#define OVERRIDE override
#elif defined(__clang__)
#define OVERRIDE override
// GCC 4.7 supports explicit virtual overrides when C++11 support is enabled.
#define OVERRIDE override
#else
#define OVERRIDE
#endif

This allows code to be compiled with C++11 compliant compilers and get warnings and errors that clang, MSVC,gcc can give, while still allowing other legacy pre C++11 compilers to compile the code. This can be an important step towards modernizing C++ code whilst living in a legacy codebase.

When it comes to clang tidy, the use of the modernize-use-override is one of the most useful checks, but the messages reported are inaccurate for that codebase if the standard approach is to use the macros OVERRIDE and/or FINAL.

When combined with fix-its that introduce the C++11 override keyword, they become fatal, resulting in the modernize-use-override check being turned off to prevent the introduction of such errors.

This revision, allows the possibility for the replacement override to be a macro instead, Allowing the clang-tidy check to be run on both pre and post C++11 code, and allowing fix-its to be applied.

Diff Detail

Repository
rL LLVM

Event Timeline

MyDeveloperDay created this revision.Jan 23 2019, 1:20 AM

Adding release note change

alexfh requested changes to this revision.Jan 23 2019, 4:50 AM

I tend to think that a better migration strategy is to change the compiler to a C++11-compatible one first, and then turn on C++11 mode and migrate the code (possibly file-by-file or with a different granularity). But if you observe a situation where compatibility macros for C++11 constructs are actually a better way to migrate, then the proposed functionality makes sense.

clang-tidy/modernize/UseOverrideCheck.cpp
32 ↗(On Diff #183063)

I'd suggest to default to an empty string and use override as a fallback right in the code where the diagnostic is generated.

42–46 ↗(On Diff #183063)

I think, this should be left as is, because

  1. clang-tidy understands C++11 and it makes sense to run it in C++11 mode to get more information out of compiler (e.g. then OVERRIDE macro will actually be defined to override and clang-tidy wouldn't have to jump through the hoops to detect that a method is already declared override).
  2. I've seen folks who just #define override in pre-C++11 mode to make the code compile.
97 ↗(On Diff #183063)

The utility of the function is questionable. I'd drop it and replace the call with if (!OverrideMacro.empty() && !Context.Idents.get(OverrideMacro).hasMacroDefinition()) ....

121–131 ↗(On Diff #183063)

How about using diagnostic arguments instead of string concatenation (diag(Loc, "prefer using %1 or %2") << Macro1 << Macro2;)?

test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
95 ↗(On Diff #183063)

Please add a test where the OVERRIDE macro is already present. Same for the FINAL macro.

This revision now requires changes to proceed.Jan 23 2019, 4:50 AM
JonasToth added inline comments.Jan 23 2019, 7:43 AM
clang-tidy/modernize/UseOverrideCheck.cpp
133 ↗(On Diff #183063)

Dangling? These values seem to be temporary and StringRef would bind to the temporary, not?
For the concatenation llvm::Twine would be better as well, same in the other places.

I tend to think that a better migration strategy is to change the compiler to a C++11-compatible one first, and then turn on C++11 mode and migrate the code (possibly file-by-file or with a different granularity). But if you observe a situation where compatibility macros for C++11 constructs are actually a better way to migrate, then the proposed functionality makes sense.

@alexfh, I couldn't agree more, however unfortunately if you are having to support a common code base which also supports older platforms like HPUX, Solaris, AIX even getting a C++11 compiler can be more of a challenge! Those platforms have expensive commercial compilers but are often lacking behind the standards. If your lucky you can find a gcc that will work but it tends to be a low version one. And then building a later gcc on those platforms is often a challenge too, even if you can get it and your code to build you often meet other hard to find issues with the final binary.

This is similar to the discussion about getting clang to compile with a minimum C++14 or C++17 compiler, it can be hard to pull the toolchain up to the level where all the supported platforms still work.

Having said all this I appreciate the code review comments, let me take a look at rewriting the bits you highlighted...which I totally agree with and think it makes for a cleaner solution. (I was trying not alter the code too much around line 120)

MyDeveloperDay marked 12 inline comments as done.

Addressing review comments,

  • reduce changes causing excessive string concats and problems with temporaries
  • simplify cxx version and macro checks
clang-tidy/modernize/UseOverrideCheck.cpp
32 ↗(On Diff #183063)

So I tried this and and met with some issues with the unit tests where it seemed to think "override" was a macro, I also found myself just simply always setting OverrideMacro/Final Macro to "override" and "final" anyway.. I've changed this round a little to only check for the macro when the OverrideMacro isn't override. This seems to resolve the problem, let me know if it still feels wrong.

42–46 ↗(On Diff #183063)

I agree with this..I don't need to run clang-tidy in cxx98 mode..reverting to the original check

97 ↗(On Diff #183063)

removed the function, but did change the condition here to be OverrideMacro!="override" to overcome issue in the unit test when not overriding the macro it would sometimes return here and not perform the fix-it, I wondered if check_clang_tidy.py -fix somehow ran the clang tidy check() function twice, once for the message and once for the fix.. but I didn't dig in any further.

121–131 ↗(On Diff #183063)

switched to using %0 and %1 hope that is correct, that 0 vs 1 stumped me for a bit, but I looked at other checks doing the same thing

133 ↗(On Diff #183063)

removed the need for so may concatenations, I hope by using %0 and %1

test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
95 ↗(On Diff #183063)

added a couple more..let me know if its sufficient.. changed the name of the test to remove cxx98 too

alexfh requested changes to this revision.Feb 15 2019, 5:51 AM
alexfh added inline comments.
clang-tidy/modernize/UseOverrideCheck.cpp
32 ↗(On Diff #183063)

In case "override" is not a macro, setting OverrideMacro to override would be somewhat confusing. We could make set default to override, if this makes logic simpler, but then I'd suggest to rename the option to OverrideSpelling (same for final).

docs/clang-tidy/checks/modernize-use-override.rst
9 ↗(On Diff #183268)

Please enclose "virtual" in double backquotes.

13 ↗(On Diff #183268)

override and final keywords were introduced ...

14 ↗(On Diff #183268)

s/There/Their/

39 ↗(On Diff #183268)

Enclose override in double backquotes.

test/clang-tidy/modernize-use-override-with-macro.cpp
2 ↗(On Diff #183268)

"modernize-use-nodiscard"? Does this test pass?

test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
2 ↗(On Diff #183268)

Same here.

16 ↗(On Diff #183268)

The check should still issue a warning here, imo. Maybe without a fix, but it should draw attention to the issue.

This revision now requires changes to proceed.Feb 15 2019, 5:51 AM
MyDeveloperDay marked 10 inline comments as done.

Address review comments

  • change OverrideMacro to OverrideSpelling
  • change FinalMacro to FinalSpelling
  • fix unit tests
  • show warning without fix-it if Macros are not defined
clang-tidy/modernize/UseOverrideCheck.cpp
32 ↗(On Diff #183063)

I agree that is a better naming

test/clang-tidy/modernize-use-override-with-macro.cpp
2 ↗(On Diff #183268)

my bad I was having problems with getting lit to work in windows environment, so was running the tests externally, I've fixed that now

c:/clang/llvm/build/RelWithDebInfo/bin/llvm-lit.py -v test/clang-tidy/modernize-use-override*
-- Testing: 5 tests, 5 threads --
PASS: Clang Tools :: clang-tidy/modernize-use-override-cxx98.cpp (1 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-ms.cpp (2 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-with-no-macro-inscope.cpp (3 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override.cpp (4 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-with-macro.cpp (5 of 5)
Testing Time: 5.13s
  Expected Passes    : 5

Some Clang warnings use PP.getLastMacroWithSpelling() to determine the user's macro automatically.

Some Clang warnings use PP.getLastMacroWithSpelling() to determine the user's macro automatically.

This would be a neat trick, but mainly I think users would either be using override/final or they would know what they were using say OVERRIDE/FINAL because it might be set by a convention for a code base, so maybe its better if they are explicit in their tidy options

alexfh accepted this revision.Feb 27 2019, 2:36 PM

LG. Sorry for the delay.

This revision is now accepted and ready to land.Feb 27 2019, 2:36 PM
lewmpk added a subscriber: lewmpk.Feb 28 2019, 3:31 AM
This comment was removed by lewmpk.

rebase after D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions change

as this was previous accepted I will land shortly, but I'd appreciate a quick overview... @JonasToth, @lewmpk

JonasToth accepted this revision.Feb 28 2019, 10:24 AM

LGTM

clang-tidy/modernize/UseOverrideCheck.cpp
196 ↗(On Diff #188752)

Nit: braces

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2019, 11:59 AM