Skip to content
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

ha_rocksdb.cc cleanups to minimize sysvar split diff #1470

Conversation

laurynas-biveinis
Copy link
Contributor

  • Move #include directives to the right places.
  • Factor out repeated THDVAR & ha_thd calls, & current_thd reads.
  • Use the C++11 range for loop in one place, use auto more for local variable
    declarations, add assert(false) to one can't-happen branch.
  • Reformat the source file.

@facebook-github-bot
Copy link

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

@luqun
Copy link
Contributor

luqun commented Jun 25, 2024

Hi @laurynas-biveinis, could you rebase this PR?

@facebook-github-bot
Copy link

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

@laurynas-biveinis
Copy link
Contributor Author

Hi @laurynas-biveinis, could you rebase this PR?

Sure @luqun, rebased

@facebook-github-bot
Copy link

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

@@ -17088,13 +17093,13 @@ bool ha_rocksdb::is_dd_update() const {
}

#define DEF_STATUS_VAR(name) \
{ "rocksdb_" #name, (char *)&SHOW_FNAME(name), SHOW_FUNC, SHOW_SCOPE_GLOBAL }
{"rocksdb_" #name, (char *)&SHOW_FNAME(name), SHOW_FUNC, SHOW_SCOPE_GLOBAL}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our internal linter complain this change.. it recommend to add space ..:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which clang-format version does your linter use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luqun, does it want to add spaces only to this line or to the 2nd and 3rd similar change below too?

Copy link
Contributor

@luqun luqun Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of these lines. Looks like internal linter like add a space before and after

{ "rocksdb_" #name, (char *)&SHOW_FNAME(name), SHOW_FUNC, SHOW_SCOPE_GLOBAL }

instead of

{"rocksdb_" #name, (char *)&SHOW_FNAME(name), SHOW_FUNC, SHOW_SCOPE_GLOBAL}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which clang-format version does your linter use?

clang-format --version
clang-format version 18.1.3

Copy link
Contributor

@luqun luqun Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laurynas-biveinis , are you using clang-format 19? or

If I run your change with clang-format 18, it will show following output

--- a/storage/rocksdb/ha_rocksdb.cc
+++ b/storage/rocksdb/ha_rocksdb.cc
@@ -17093,13 +17093,13 @@ bool ha_rocksdb::is_dd_update() const {
   }

 #define DEF_STATUS_VAR(name) \
-  {"rocksdb_" #name, (char *)&SHOW_FNAME(name), SHOW_FUNC, SHOW_SCOPE_GLOBAL}
+  { "rocksdb_" #name, (char *)&SHOW_FNAME(name), SHOW_FUNC, SHOW_SCOPE_GLOBAL }

 #define DEF_STATUS_VAR_PTR(name, ptr, option) \
-  {"rocksdb_" name, (char *)ptr, option, SHOW_SCOPE_GLOBAL}
+  { "rocksdb_" name, (char *)ptr, option, SHOW_SCOPE_GLOBAL }

 #define DEF_STATUS_VAR_FUNC(name, ptr, option) \
-  {name, reinterpret_cast<char *>(ptr), option, SHOW_SCOPE_GLOBAL}
+  { name, reinterpret_cast<char *>(ptr), option, SHOW_SCOPE_GLOBAL }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luqun , rebased and reformatted, ready for review

I am using clangd 19, which effectively should be clang-format 19. Interestingly if I run this file through clang-format 18.1.5 after it is already formatted with 19, the diff is empty. I have re-added the spaces manually.

@facebook-github-bot
Copy link

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

@facebook-github-bot
Copy link

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link

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

1 similar comment
@facebook-github-bot
Copy link

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

@luqun
Copy link
Contributor

luqun commented Oct 7, 2024

@laurynas-biveinis, could you also rebase this one?

- Move #include directives to the right places.
- Factor out repeated THDVAR & ha_thd calls, & current_thd reads.
- Use the C++11 range for loop in one place, use auto more for local variable
  declarations, add assert(false) to one can't-happen branch, move locals to
  narrower scopes.
- Reformat the source file.
@facebook-github-bot
Copy link

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

@laurynas-biveinis
Copy link
Contributor Author

@luqun , rebased

@facebook-github-bot
Copy link

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

@facebook-github-bot
Copy link

This pull request has been merged in 3def38f.

@laurynas-biveinis laurynas-biveinis deleted the pre-sysvars-split-cleanups branch October 16, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants