-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark virtual ~Env() override #9467
Conversation
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, and it's up to you:
should we do a sweeping update for all destructors and/or methods in public headers?
Otherwise, LGTM.
@hx235 has updated the pull request. You must reimport the pull request before landing. |
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
829ff68
to
edd072e
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
A follow up could be #9404 (comment) |
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: **Context:** Compiling RocksDB with -Winconsistent-missing-destructor-override reveals the following : ``` ./include/rocksdb/env.h:174:11: error: '~Env' overrides a destructor but is not marked 'override' [-Werror,-Winconsistent-missing-destructor-override] virtual ~Env(); ^ ./include/rocksdb/customizable.h:58:3: note: overridden virtual function is here ~Customizable() override {} ``` The need of overriding the Env's destructor seems to be introduced by facebook#9293 and surfaced by -Winconsistent-missing-destructor-override, which is not turned on by default. **Summary:** Mark ~Env() override Pull Request resolved: facebook#9467 Test Plan: - Turn on -Winconsistent-missing-destructor-override and USE_CLANG=1 make -jN env/env.o to see whether the error shows up Reviewed By: jay-zhuang, riversand963, george-reynya Differential Revision: D33864985 Pulled By: hx235 fbshipit-source-id: 4a78bd161ff153902b2676829723e9a1c33dd749 (cherry picked from commit a3de7ae)
Summary: **Context:** Compiling RocksDB with -Winconsistent-missing-destructor-override reveals the following : ``` ./include/rocksdb/env.h:174:11: error: '~Env' overrides a destructor but is not marked 'override' [-Werror,-Winconsistent-missing-destructor-override] virtual ~Env(); ^ ./include/rocksdb/customizable.h:58:3: note: overridden virtual function is here ~Customizable() override {} ``` The need of overriding the Env's destructor seems to be introduced by #9293 and surfaced by -Winconsistent-missing-destructor-override, which is not turned on by default. **Summary:** Mark ~Env() override Pull Request resolved: #9467 Test Plan: - Turn on -Winconsistent-missing-destructor-override and USE_CLANG=1 make -jN env/env.o to see whether the error shows up Reviewed By: jay-zhuang, riversand963, george-reynya Differential Revision: D33864985 Pulled By: hx235 fbshipit-source-id: 4a78bd161ff153902b2676829723e9a1c33dd749 (cherry picked from commit a3de7ae)
Context:
Compiling RocksDB with -Winconsistent-missing-destructor-override reveals the following :
The need of overriding the Env's destructor seems to be introduced by #9293 and surfaced by -Winconsistent-missing-destructor-override, which is not turned on by default.
Summary:
Mark ~Env() override
Test: