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

Remove Unused KeySet Flag #1740

Open
KurtMi opened this Issue Dec 16, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@KurtMi

KurtMi commented Dec 16, 2017

During the integration of the OPMPHM I found a unused flag, it shall be removed!

  • KS_FLAG_SYNC in kdbprivate.h:136 and the function int ksNeedSync (const KeySet * ks).

Nevermind the conversation :)

@markus2330 I told you that I need a Flag but I found another way. Whats about the second one? It also dose some weird stuff.
@mpranj could you use the first one?

@mpranj

This comment has been minimized.

Show comment
Hide comment
@mpranj

mpranj Dec 16, 2017

Contributor

I'm not sure what you mean. I haven't used KS_FLAG_SYNC for anything. I had to introduce KS_FLAG_MMAP and KEY_FLAG_MMAP so I can track what's inside a mmap region (and therefore should not be free()'d).

Contributor

mpranj commented Dec 16, 2017

I'm not sure what you mean. I haven't used KS_FLAG_SYNC for anything. I had to introduce KS_FLAG_MMAP and KEY_FLAG_MMAP so I can track what's inside a mmap region (and therefore should not be free()'d).

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 16, 2017

Contributor

Yes, one information is about who allocated the memory, the other information (@KurtMi is interested in) is about if there were changes in the KeySet.

@KurtMi Please reuse the unused KS_FLAG_SYNC (or rename it if you do not like the name). But we should definitely get rid of obsolete/historical stuff.

Contributor

markus2330 commented Dec 16, 2017

Yes, one information is about who allocated the memory, the other information (@KurtMi is interested in) is about if there were changes in the KeySet.

@KurtMi Please reuse the unused KS_FLAG_SYNC (or rename it if you do not like the name). But we should definitely get rid of obsolete/historical stuff.

@KurtMi

This comment has been minimized.

Show comment
Hide comment
@KurtMi

KurtMi Dec 18, 2017

@markus2330 I apologize for causing confusion! The point is that I do NOT need the KS_FLAG_SYNC flag and I thought @mpranj could use it. But since nobody has usage for that flag, this issue will be transformed into the task to remove the flag.

KurtMi commented Dec 18, 2017

@markus2330 I apologize for causing confusion! The point is that I do NOT need the KS_FLAG_SYNC flag and I thought @mpranj could use it. But since nobody has usage for that flag, this issue will be transformed into the task to remove the flag.

@KurtMi KurtMi changed the title from Unused Flags and Functions to Remove Unused KeySet Flag Dec 18, 2017

@mpranj

This comment has been minimized.

Show comment
Hide comment
@mpranj

mpranj Dec 18, 2017

Contributor

In that case I can reuse/rename it instead of my additional flag.

Contributor

mpranj commented Dec 18, 2017

In that case I can reuse/rename it instead of my additional flag.

@KurtMi KurtMi assigned mpranj and unassigned KurtMi Dec 18, 2017

@KurtMi

This comment has been minimized.

Show comment
Hide comment
@KurtMi

KurtMi Jun 13, 2018

I found out that removing that flag appears to be non trivial since the function ksNeedSync (...), that reads out that flag is also present in the bindings:

src/bindings/haskell/src/Elektra/KeySet.chs:12:  ksGetSize, ksNeedSync,
src/bindings/haskell/src/Elektra/KeySet.chs:55:{#fun unsafe ksNeedSync {`KeySet'} -> `Int' #}
src/bindings/jna/libelektra4j/src/main/java/org/libelektra/Elektra.java:143:    int ksNeedSync(Pointer ks);
src/bindings/jna/libelektra4j/src/main/java/org/libelektra/KeySet.java:175:             return Elektra.INSTANCE.ksNeedSync(get());

KurtMi commented Jun 13, 2018

I found out that removing that flag appears to be non trivial since the function ksNeedSync (...), that reads out that flag is also present in the bindings:

src/bindings/haskell/src/Elektra/KeySet.chs:12:  ksGetSize, ksNeedSync,
src/bindings/haskell/src/Elektra/KeySet.chs:55:{#fun unsafe ksNeedSync {`KeySet'} -> `Int' #}
src/bindings/jna/libelektra4j/src/main/java/org/libelektra/Elektra.java:143:    int ksNeedSync(Pointer ks);
src/bindings/jna/libelektra4j/src/main/java/org/libelektra/KeySet.java:175:             return Elektra.INSTANCE.ksNeedSync(get());
@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Jun 13, 2018

Contributor

Yes, additionally it would be an ABI/API break in the core library. So it needs to wait to 1.0 anyway.

doc/todo/FUTURE already lists ksNeedSync to be removed.

But: always returning true/false (like it was before) should be fine, too. Then we could remove the flag, and only keep the ksNeedSync

Contributor

markus2330 commented Jun 13, 2018

Yes, additionally it would be an ABI/API break in the core library. So it needs to wait to 1.0 anyway.

doc/todo/FUTURE already lists ksNeedSync to be removed.

But: always returning true/false (like it was before) should be fine, too. Then we could remove the flag, and only keep the ksNeedSync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment