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

Warn if result of ts_set_flags_32 is not used #5856

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

jnidzwetzki
Copy link
Contributor

@jnidzwetzki jnidzwetzki commented Jul 4, 2023

The ts_set_flags_32 function takes a bitmap and flags and returns an updated bitmap. However, if the returned value is not used, the function call has no effect. An unused result may indicate the improper use of this function.

This patch adds the qualifier pg_nodiscard to the function which triggers a warning if the returned value is not used.

Disable-check: force-changelog-file

The ts_set_flags_32 function takes a bitmap and flags and returns an
updated bitmap. However, if the returned value is not used, the function
call has no effect. An unused result may indicate the improper use of this
function.

This patch adds the qualifier pg_nodiscard to the function which
triggers a warning if the returned value is not used.
@jnidzwetzki jnidzwetzki marked this pull request as ready for review July 4, 2023 14:23
@github-actions github-actions bot requested review from konskov and pmwkaa July 4, 2023 14:24
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

@konskov, @pmwkaa: please review this pull request.

Powered by pull-review

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #5856 (e55bf0d) into main (06d20b1) will decrease coverage by 7.51%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5856      +/-   ##
==========================================
- Coverage   87.87%   80.37%   -7.51%     
==========================================
  Files         239      239              
  Lines       55742    49014    -6728     
  Branches    12350    12289      -61     
==========================================
- Hits        48985    39396    -9589     
+ Misses       4864     4217     -647     
- Partials     1893     5401    +3508     
Impacted Files Coverage Δ
src/utils.h 73.52% <ø> (-9.81%) ⬇️

... and 222 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +172 to 175
static inline pg_nodiscard uint32
ts_set_flags_32(uint32 bitmap, uint32 flags)
{
return bitmap | flags;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just replace it with |=?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about modifying the value directly. But then decided against it to have a function signature that is similar to other PostgreSQL functions like lappend().

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean, remove the function altogether. Anyway, with your change it's better.

@jnidzwetzki jnidzwetzki enabled auto-merge (rebase) July 5, 2023 07:09
@jnidzwetzki jnidzwetzki merged commit 490bc91 into timescale:main Jul 5, 2023
42 checks passed
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

3 participants