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

Refine ST_FLAG_NUMBER-related behaviours #357

Closed
wants to merge 4 commits into from

Conversation

zykh
Copy link
Contributor

@zykh zykh commented Jan 6, 2017

Follow-up to #265/#267:

  • explicitly setting ST_FLAG_NUMBER for ranges through dstate_setflags() causes the elimination of previously-set flags, inducing some side effects (e.g. if a var is first set as r/w and then its range is added -> the var loses its r/w status and becomes unchangeable): this PR introduces a support function to add flags (and its counterpart to remove flags) without losing the existing ones.
  • also, a couple of remaining bits were not updated for ST_FLAG_NUMBER.

Ranges are now explicitly flagged as numbers, but the use of dstate_setflags() means that we substitute any previously-set flag with the ones we pass to that function, losing, for example, an already-set r/w status, with the result of making an affected var unchangeable.
Use instead the newly added dstate_addflags() function, which preserves already-set flags.
@clepple
Copy link
Member

clepple commented Jan 10, 2017

I will admit that I have not been following this very closely, and I think that eventually we should have some test cases that exercise this code. But given the general lack of C unit-test infrastructure, don't let that stop you from merging.

@aquette, any thoughts?

@aquette aquette mentioned this pull request Jan 10, 2017
@aquette
Copy link
Member

aquette commented Jan 10, 2017

@clepple fully agree with the need to improve tests in this area (was thinking the exact same yesterday).
I've logged an issue to track that.
As for the current issue, I was not able to test, so @zykh if you made the necessary test (and I'm sure you did), I trust you, so don't hesitate to go ahead and merge.

@zykh
Copy link
Contributor Author

zykh commented Jan 11, 2017

Thanks to all, rebased and merged.

@zykh zykh closed this Jan 11, 2017
@zykh zykh deleted the range-number branch January 11, 2017 00:06
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