modernize-use-override: ignore when `virtual` is from a macro

[saschanaz]

Mozilla uses NS_IMETHOD macro a lot to implement a certain kind of virtual methods.

#ifdef XP_WIN
#  define NS_IMETHOD_(type) virtual type __stdcall
#else
#  define NS_IMETHOD_(type) virtual type
#endif

#define NS_IMETHOD NS_IMETHOD_(nsresult)

We use this to implement both a base method and overrides:

class Foo {
  NS_IMETHOD Foo();
};

class Bar final : public Foo {
  NS_IMETHOD Foo() override;
};

modernize-use-override whines at this kind of code:

'virtual' is redundant since the function is already declared 'override' [modernize-use-override]

It's redundant, yes, but that's okay as it's hidden behind the macro. I think the linter should ignore such case.

The fix would be to check actual token before warning about redundancy, as the hint generation lines do:

https://github.com/llvm/llvm-project/blob/40546cb38189e438a81faa6400c103d159600f9e/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp#L220-L228

See also:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1420366
• https://bugzilla.mozilla.org/show_bug.cgi?id=1756070#c17

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[njames93]

I'm not sure this is really a use case that should be supported, take this:

#define VIRTUAL virtual

struct Foo {
  VIRTUAL int getID() const;
};

struct Bar : Foo {
  VIRTUAL int getID() override;
};

In this case I'd argue we definitely should have a warning. Maybe there could be merit in your case when the macro isn't just for defining the virtual keyword

[saschanaz]

Yes, it's more than just a single-keyword macro. But even it was a single-keyword one, I think something like this could still make sense:

#define IMETHODDECL virtual
struct Foo {
  IMETHODDECL void foo();
};
struct Bar : Foo {
  IMETHODDECL void foo() override; // it's probably there for a reason, maybe for some style guide.
};

[emilio]

It'd be somewhat weird, I think, to have a macro that expands only to the virtual keyword, right?

There are a few other warnings that are silenced as a result of macro expansion, iirc (though on the phone, can try to look for precedents later).

[saschanaz]

Haven't checked yet, but I see quite some amount of isMacroID() checks in clang-tidy.

[njames93]

Haven't checked yet, but I see quite some amount of isMacroID() checks in clang-tidy.

Those are often there as FixIts in macro code often causes compilation issues

[saschanaz]

I see such warn-but-no-fix cases (just like modernize-use-override) but also no-warn-at-all ones:

https://github.com/llvm/llvm-project/blob/4a097efe7784767b7d12ffcb8f2b22b9f4d045e2/clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp#L62-L78

[AaronBallman]

We often will suppress diagnostics in the presence of macros (same as fix-its) because macros make it very difficult to know whether the user has the ability to silence the diagnostic themselves or not due to configuration differences. I think this is one of those cases -- Mozilla uses this in a number of places, and that's indicative of a code pattern that exists in the wild where this otherwise useful check has to be disabled. That may be worth considering adding a configuration option over it. That said, I definitely think it's a questionable to hide the virtual behind a macro as that drastically impacts the intent of the interface when the macro is disabled.

[saschanaz]

That said, I definitely think it's a questionable to hide the virtual behind a macro as that drastically impacts the intent of the interface when the macro is disabled.

Good question-able. We want to make sure every NS_IMETHOD be virtual, that's why it's written like that in Mozilla's case.