Skip to content

remove consts from tsk_json_struct_metadata_get_blob (closes #3425)#3427

Open
petrelharp wants to merge 1 commit intotskit-dev:mainfrom
petrelharp:noconst
Open

remove consts from tsk_json_struct_metadata_get_blob (closes #3425)#3427
petrelharp wants to merge 1 commit intotskit-dev:mainfrom
petrelharp:noconst

Conversation

@petrelharp
Copy link
Contributor

As discussed in #3425, it is maybe helpful to remove the const from some of the arguments to tsk_json_struct_metadata_get_blob.

My superficial understanding of the issue is as follows: suppose you have a function that accepts a pointer, say *blob, and doesn't modify the stuff that pointer points at. Then it's fine to define your function to accept const foo *blob but then pass it in a non-const foo *blob from somewhere else, since const is supposed to mean "we don't change this here" not "we don't change this anywhere". However if it takes a pointer-to-a-pointer, say const foo **blob then there are ways that you'd actually be able to modify the contents being ultimately pointed at without, like, noticing. So it is not cool to pass in foo **blob.

So, I agree - seems like we should indeed remove the const.

According to this page the "most common solution" is to declare it like const foo* const *blob; however, I think this means you can't change the pointer *blob either and we do indeed need to do that in this function. And I tried it and it doesn't seem to work.

So here I've removed the const from **blob. This meant I also needed to remove const from **json and *metadata, since these are all pointing at the same chunk of memory.

@codecov
Copy link

codecov bot commented Mar 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.92%. Comparing base (1835ea3) to head (c531bfa).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3427   +/-   ##
=======================================
  Coverage   91.92%   91.92%           
=======================================
  Files          37       37           
  Lines       32153    32153           
  Branches     5143     5143           
=======================================
  Hits        29556    29556           
  Misses       2264     2264           
  Partials      333      333           
Flag Coverage Δ
C 82.70% <100.00%> (ø)
c-python 77.34% <0.00%> (ø)
python-tests 96.40% <ø> (ø)
python-tests-no-jit 33.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Python API 98.69% <ø> (ø)
Python C interface 91.23% <ø> (ø)
C library 88.86% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jeromekelleher jeromekelleher added this pull request to the merge queue Mar 12, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 12, 2026
@bhaller
Copy link

bhaller commented Mar 12, 2026

I'm not sure what this means: "github-merge-queue bot removed this pull request from the merge queue due to no response for status checks". But it looks like it passed all checks, and the PR looks good to me. Can this merge be bumped somehow?

@petrelharp petrelharp added this pull request to the merge queue Mar 12, 2026
@petrelharp
Copy link
Contributor Author

I suspect maaaaybe github's automated stuff dropped the ball? Trying again; will manually merge if not.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 12, 2026
@bhaller
Copy link

bhaller commented Mar 12, 2026

I suspect maaaaybe github's automated stuff dropped the ball? Trying again; will manually merge if not.

The auto-merge seems to have failed again. Maybe there's a time limit for all the checks to complete, and if they don't finish within that time limit the auto-merge gets cancelled? In which case that time limit needs to get extended somehow/somewhere, otherwise the auto-merge is seeming pretty useless. :-> Anyhow, manual merge for now, I guess?

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.

3 participants