Conditionally declare and define variable that is unused in LITE mode #9854

hx235 · 2022-04-17T06:24:47Z

Context:
As mentioned in #9701, we have the following in LITE=1 make static_lib for v7.0.2

  CC       file/sequence_file_reader.o
  CC       file/sst_file_manager_impl.o
  CC       file/writable_file_writer.o
In file included from file/writable_file_writer.cc:10:
./file/writable_file_writer.h:163:15: error: private field 'temperature_' is not used [-Werror,-Wunused-private-field]
  Temperature temperature_;
              ^
1 error generated.
make: *** [file/writable_file_writer.o] Error 1

Summary: as titled

Test plan:

Local LITE=1 make static_lib reveals the same error and error is gone after this fix
CI

hx235 · 2022-04-17T06:35:42Z

@ajkr in this case with a user issue, should we backport this fix to 7.0 and 7.1 branch (making a new minor version with a new tag for each of the branches) and potentially for 7.2 if this PR did not make it in the 7.2?

hx235 · 2022-04-17T06:50:29Z

file/writable_file_writer.h

@@ -160,7 +160,9 @@ class WritableFileWriter {
  bool perform_data_verification_;
  uint32_t buffered_data_crc32c_checksum_;
  bool buffered_data_with_checksum_;
+#ifndef ROCKSDB_LITE


Simply adding unused or maybe-used attributes will trigger -Werror=attributes in LITE=1 make static_lib, like below:

./file/writable_file_writer.h:163:31: error: ‘maybe_unused’ attribute ignored [-Werror=attributes] 163 | [[maybe_unused]]Temperature temperature_;

Not sure whether it's okay to remove -Werror=attributes in LITE=1 make static_lib build so I decide to go for a more verbose way (conditionally declaration and definition)

another option is using (void)temperature_ in LITE.

facebook-github-bot · 2022-04-17T07:13:04Z

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mrambacher

LGTM. Please consider the nit.

mrambacher · 2022-04-17T11:38:14Z

file/writable_file_writer.h

-        buffered_data_with_checksum_(buffered_data_with_checksum),
-        temperature_(options.temperature) {
+        buffered_data_with_checksum_(buffered_data_with_checksum)
+#ifndef ROCKSDB_LITE


It is a little cleaner (code appearance-wise) if you just set temperature_ in the constructor body conditionally rather than as an argument initializer. Another option would be to move the temperature_ to not be the last item to be initialized.

It is a minor nit, but will make code maintenance simpler going forward if new arguments/options are added.

Good point- will fix it

jay-zhuang

LGTM, thanks for the fix.

jay-zhuang · 2022-04-18T01:02:32Z

file/writable_file_writer.h

@@ -160,7 +160,9 @@ class WritableFileWriter {
  bool perform_data_verification_;
  uint32_t buffered_data_crc32c_checksum_;
  bool buffered_data_with_checksum_;
+#ifndef ROCKSDB_LITE


another option is using (void)temperature_ in LITE.

ajkr

LGTM

@ajkr in this case with a user issue, should we backport this fix to 7.0 and 7.1 branch (making a new minor version with a new tag for each of the branches) and potentially for 7.2 if this PR did not make it in the 7.2?

Sounds good. I'd skip the 7.0 tag unless requested.

facebook-github-bot · 2022-04-18T18:39:39Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-04-18T18:39:49Z

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

…facebook#9854) Summary: Context: As mentioned in facebook#9701, we have the following in LITE=1 make static_lib for v7.0.2 ``` CC file/sequence_file_reader.o CC file/sst_file_manager_impl.o CC file/writable_file_writer.o In file included from file/writable_file_writer.cc:10: ./file/writable_file_writer.h:163:15: error: private field 'temperature_' is not used [-Werror,-Wunused-private-field] Temperature temperature_; ^ 1 error generated. make: *** [file/writable_file_writer.o] Error 1 ``` as titled Pull Request resolved: facebook#9854 Test Plan: - Local `LITE=1 make static_lib` reveals the same error and error is gone after this fix - CI Reviewed By: ajkr, jay-zhuang Differential Revision: D35706585 Pulled By: hx235 fbshipit-source-id: 7743310298231ad6866304ffa2225c8abdc91d9a

…#9854) Summary: Context: As mentioned in #9701, we have the following in LITE=1 make static_lib for v7.0.2 ``` CC file/sequence_file_reader.o CC file/sst_file_manager_impl.o CC file/writable_file_writer.o In file included from file/writable_file_writer.cc:10: ./file/writable_file_writer.h:163:15: error: private field 'temperature_' is not used [-Werror,-Wunused-private-field] Temperature temperature_; ^ 1 error generated. make: *** [file/writable_file_writer.o] Error 1 ``` as titled Pull Request resolved: #9854 Test Plan: - Local `LITE=1 make static_lib` reveals the same error and error is gone after this fix - CI Reviewed By: ajkr, jay-zhuang Differential Revision: D35706585 Pulled By: hx235 fbshipit-source-id: 7743310298231ad6866304ffa2225c8abdc91d9a

…facebook#9854) Summary: Context: As mentioned in facebook#9701, we have the following in LITE=1 make static_lib for v7.0.2 ``` CC file/sequence_file_reader.o CC file/sst_file_manager_impl.o CC file/writable_file_writer.o In file included from file/writable_file_writer.cc:10: ./file/writable_file_writer.h:163:15: error: private field 'temperature_' is not used [-Werror,-Wunused-private-field] Temperature temperature_; ^ 1 error generated. make: *** [file/writable_file_writer.o] Error 1 ``` as titled Pull Request resolved: facebook#9854 Test Plan: - Local `LITE=1 make static_lib` reveals the same error and error is gone after this fix - CI Reviewed By: ajkr, jay-zhuang Differential Revision: D35706585 Pulled By: hx235 fbshipit-source-id: 7743310298231ad6866304ffa2225c8abdc91d9a

facebook-github-bot added the CLA Signed label Apr 17, 2022

hx235 force-pushed the lite_unused_var branch from 8bba18c to 7b549b0 Compare April 17, 2022 06:33

hx235 changed the title ~~Add unused attribute to a unused variable in LITE mode~~ Conditionally declare and define variable that is unused in LITE mode Apr 17, 2022

hx235 force-pushed the lite_unused_var branch 2 times, most recently from 595ef5d to 8d75539 Compare April 17, 2022 06:42

hx235 commented Apr 17, 2022

View reviewed changes

hx235 force-pushed the lite_unused_var branch from 8d75539 to 03daaf5 Compare April 17, 2022 06:56

Conditionally declare and define unused var in LITE mode

dd4c13f

hx235 force-pushed the lite_unused_var branch from 03daaf5 to dd4c13f Compare April 17, 2022 06:57

mrambacher approved these changes Apr 17, 2022

View reviewed changes

jay-zhuang approved these changes Apr 18, 2022

View reviewed changes

ajkr approved these changes Apr 18, 2022

View reviewed changes

Nit: initialize in constructor body

7b9d6b8

facebook-github-bot closed this in e83c554 Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditionally declare and define variable that is unused in LITE mode #9854

Conditionally declare and define variable that is unused in LITE mode #9854

hx235 commented Apr 17, 2022 •

edited

Loading

hx235 commented Apr 17, 2022 •

edited

Loading

hx235 Apr 17, 2022 •

edited

Loading

jay-zhuang Apr 18, 2022

facebook-github-bot commented Apr 17, 2022

mrambacher left a comment

mrambacher Apr 17, 2022

hx235 Apr 17, 2022

hx235 Apr 18, 2022

jay-zhuang left a comment

jay-zhuang Apr 18, 2022

ajkr left a comment

facebook-github-bot commented Apr 18, 2022

facebook-github-bot commented Apr 18, 2022

Conditionally declare and define variable that is unused in LITE mode #9854

Conditionally declare and define variable that is unused in LITE mode #9854

Conversation

hx235 commented Apr 17, 2022 • edited Loading

hx235 commented Apr 17, 2022 • edited Loading

hx235 Apr 17, 2022 • edited Loading

Choose a reason for hiding this comment

jay-zhuang Apr 18, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Apr 17, 2022

mrambacher left a comment

Choose a reason for hiding this comment

mrambacher Apr 17, 2022

Choose a reason for hiding this comment

hx235 Apr 17, 2022

Choose a reason for hiding this comment

hx235 Apr 18, 2022

Choose a reason for hiding this comment

jay-zhuang left a comment

Choose a reason for hiding this comment

jay-zhuang Apr 18, 2022

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 18, 2022

facebook-github-bot commented Apr 18, 2022

hx235 commented Apr 17, 2022 •

edited

Loading

hx235 commented Apr 17, 2022 •

edited

Loading

hx235 Apr 17, 2022 •

edited

Loading