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

cmake: enable Wall #2541

Merged
merged 1 commit into from
Dec 4, 2023
Merged

cmake: enable Wall #2541

merged 1 commit into from
Dec 4, 2023

Conversation

Riolku
Copy link
Collaborator

@Riolku Riolku commented Dec 4, 2023

As pointed out in #2536, -Wall is not enabled. -Wextra will follow in a separate PR.

The main changes needed are:

  • Consistent use of either struct/class for a symbol, not a mix.
  • Don't call std::move() on temporaries, the compiler will elide the copy anyway.
  • Constructor initialization order should be the same as the member order. It turns out that C++ will always initialize the fields in the member order, regardless of the initialization list ordering.
  • Mark functions in header files as inline. Inline static doesn't really make sense it seems, and Apple mistakenly warns about it. Inline functions already have no linkage.

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (7e3890b) 92.86% compared to head (03607fb) 92.85%.
Report is 1 commits behind head on master.

Files Patch % Lines
...e/binder/query/updating_clause/bound_delete_info.h 33.33% 2 Missing ⚠️
src/storage/storage_structure/disk_array.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2541      +/-   ##
==========================================
- Coverage   92.86%   92.85%   -0.02%     
==========================================
  Files        1025     1025              
  Lines       38342    38338       -4     
==========================================
- Hits        35608    35597      -11     
- Misses       2734     2741       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Riolku Riolku force-pushed the enable-more-warnings branch 3 times, most recently from c01e1b9 to e7f9853 Compare December 4, 2023 18:15
As pointed out in #2536, -Wall is not enabled. -Wextra will follow in a
separate PR.

The main changes needed are:
- Consistent use of either struct/class for a symbol, not a mix.
- Don't call `std::move()` on temporaries, the compiler will elide the
  copy anyway.
- Constructor initialization order should be the same as the member
  order. It turns out that C++ will always initialize the fields in the
  member order, regardless of the initialization list ordering.
- Mark functions in header files as inline. Inline static doesn't really
  make sense it seems, and Apple mistakenly warns about it. Inline
  functions already have no linkage.
@Riolku Riolku merged commit 47a50c1 into master Dec 4, 2023
14 checks passed
@Riolku Riolku deleted the enable-more-warnings branch December 4, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants