Open Bug 1756070 Opened 4 years ago Updated 2 years ago

Reenable modernize-use-override

Categories

(Developer Infrastructure :: Source Code Analysis, task)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
task

Tracking

(Not tracked)

Status:
ASSIGNED

People

(Reporter: saschanaz, Assigned: saschanaz)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 14 obsolete files)

Bug 1756070 - Part 1: Add missing `override` keywords r=andi
48 bytes, text/x-phabricator-request
Details | Review
Bug 1756070 - Part 2: Apply modernize-use-override to dom/file r=smaug
48 bytes, text/x-phabricator-request
Details | Review

https://searchfox.org/mozilla-central/search?q=NS_IMETHOD%28_%5C%28%5B%5E%29%5D%2B%5C%29%29%3F+%5B%5E%29%5D%2B%5C%29%28+%3D+0%7C+const%29%3F%28%3B%7C+%5C%7B%29&path=&case=false&regexp=true

Quite some matches but most of them are generated by parser and should not be hard to fix.

By fix I mean splitting NS_IMETHOD into NS_IMETHOD_BASE (with virtual) and NS_IMETHOD (without virtual).

Hi Nika, NI'ing you as this touches XPCOM.

The intention is to split virtual keyword out of NS_IMETHOD to make our code compatible with modernize-use-override (which is basically "use virtual or override but not both").

Current:

class Foo {
  NS_IMETHOD Bar();
};

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

And my patches:

class Foo {
  virtual NS_IMETHOD Bar();
};

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

Please ping me if you have some concerns, thank you!

Flags: needinfo?(nika)

It feels pretty weird that we'd allow non-virtual NS_IMETHOD. Can we instead fix the lint so that it doesn't complain about keywords coming from macro expansion?

"Fix the lint" or maybe "add the lint" to force NS_IMETHOD be virtual? (I believe modifying a clang builtin lint is not very recommended and I don't think it's doing wrong)

The earlier version of my patches used #define NS_IMETHOD_BASE virtual NS_IMETHOD, that should also work. (Or maybe not...)

(In reply to Kagami :saschanaz from comment #18)

"Fix the lint" or maybe "add the lint" to force NS_IMETHOD be virtual?

I mean, that'd be another option I guess.

(I believe modifying a clang builtin lint is not very recommended and I don't think it's doing wrong)

There's tons of clang warnings that behave differently to support macro expansion IIRC, so maybe such a thing would be acceptable upstream.

Hmm, in that case I'll at least file an issue.

Attachment #9264635 - Attachment is obsolete: true
Attachment #9264636 - Attachment is obsolete: true
Attachment #9264637 - Attachment is obsolete: true
Attachment #9264638 - Attachment is obsolete: true
Attachment #9264639 - Attachment is obsolete: true
Attachment #9264640 - Attachment is obsolete: true
Attachment #9264641 - Attachment is obsolete: true
Attachment #9264643 - Attachment is obsolete: true
Attachment #9264644 - Attachment is obsolete: true
Attachment #9264645 - Attachment is obsolete: true
Attachment #9264646 - Attachment is obsolete: true
Attachment #9264647 - Attachment is obsolete: true
Attachment #9264707 - Attachment is obsolete: true
Attachment #9264642 - Attachment is obsolete: true

Okay, I now think comment #17 makes sense, actually a lot. Filed https://github.com/llvm/llvm-project/issues/53949. Feel free to add some comment if something is missing, thanks!

Flags: needinfo?(nika)
See Also: → https://github.com/llvm/llvm-project/issues/38276
Summary: Fix NS_IMETHOD and reenable modernize-use-override → Reenable modernize-use-override
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cbe30b080d1b Part 1: Add missing `override` keywords r=emilio

Long time no activity there. We might want to copy-paste their rule and tweak it.

Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40a558cf1ff3 Part 2: Apply modernize-use-override to dom/file r=smaug
Product: Firefox Build System → Developer Infrastructure

The leave-open keyword is there and there is no activity for 6 months.
:saschanaz, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(krosylight)
Flags: needinfo?(krosylight)
Keywords: leave-open

Removing NS_IMETHOD would also allow this, but per nika we can't do that before some assembly expert adjust stdcall specific parts. Bug 662348 has a patch but I don't think I can review that.

Depends on: 662348
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: