add ksSearch to public API

[markus2330]

As we want external iteration as the only iteration it probably makes sense to put elektraCursor ksSearch(KeySet*, Key*) into the public API: it returns the cursor of a key name: as ksLookup, only the return types differs.

There is already a use case: #4002.

@lawli3t Do you think this would be still a minimal API?

Like always: everyone can share his/her thoughts about this proposal.

[kodebach]

I agree that ksSearch could be useful. Regarding "minimal API": We could change ksLookup to be a thin wrapper around ksSearch.

static inline Key * ksLookup (KeySet * ks, Key * k, elektraLookupFlags options)
{
    elektraCursor cursor = ksSearch (ks, k);
    if (cursor < 0) return cursor;
    return ksAtCursor (cursor);
}

All elektraLookupFlags options should work with ksSearch, except for KDB_O_POP. IMO removing KDB_O_POP and requiring a separate call to something like ksRemove (KeySet *, elektraCursor) would be fine. It would certainly be more self-explanatory than the KDB_O_POP option.

Another benefit of making ksSearch the main API is that it highlights the ordered nature of a KeySet.

[markus2330]

except for KDB_O_POP

• KDB_O_CREATE also does not work
• KDB_O_DEL was probably never a good idea (ksLookupByName is superior)
• KDB_O_SPEC is now irrelevant as we do not want spec keys in the keyset anyway

So which flag remains which you think it is important for ksSearch?

Probably we should even get rid of the argument from ksLookup? (and provide ksRemove instead)

[lawli3t]

Seems like the best option going forward Markus Raab ***@***.***> schrieb am Fr., 10. Sept. 2021, 18:05:

…

except for KDB_O_POP - KDB_O_CREATE also does not work - KDB_O_DEL was probably never a good idea (ksLookupByName is superior) - KDB_O_SPEC is now irrelevant as we do not want spec keys in the keyset anyway So which flag remains which you think it is important for ksSearch? Probably we should even get rid of the argument from ksLookup? (and provide ksRemove instead) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4026 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFR66CJTWWZNQGFLDF6P63UBIUFFANCNFSM5DOVS7RQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[kodebach]

KDB_O_CREATE also does not work

Why? ksSearch could still create and insert the key and then return the cursor for the new key.

KDB_O_DEL was probably never a good idea

I have used it once or twice, but adding another keyDel call wouldn't be too hard.

Probably we should even get rid of the argument from ksLookup? (and provide ksRemove instead)

Probably we should have a remove method that takes a cursor (called ksRemoveAt below, for lack of a better name). Then we can implement ksRemove as:

static inline Key * ksRemove (KeySet * ks, Key * k)
{
    return ksRemoveAt (ks, ksSearch (ks, k));
}

[markus2330]

Why?

Actually it also applies to ksLookup: #4034

Probably we should have a remove method that takes a cursor (called ksRemoveAt below, for lack of a better name).

It makes sense that we offer a full iterator-based interface (including removal) and get rid all the special features from ksLookup (like creation and removal).

called ksRemoveAt below

ksRemoveAtCursor would be more consistent to ksAtCursor.

[tucek]

As discussed with @markus2330, I will add ksSearch to the public C API. ksSearch will first call elektraLookupOpmphmSearch and then fallback on ksSearchInternal.
ksSearchInternal will be made private (src/libs/elektra/symbols.map) and new ksSearch will be public.

[kodebach]

ksSearchInternal will be made private (src/libs/elektra/symbols.map)

Is this necessary? I don't think there will be a need to call ksSearchInternal outside of libelektra-core.

[tucek]

Optimization of ksSearch by first searching hashmap is extracted to issue #4039

[markus2330]

I will add ksSearch to the public C API

Thank you 👍

ksSearchInternal will be made private (src/libs/elektra/symbols.map)
Is this necessary? I don't think there will be a need to call ksSearchInternal outside of libelektra-core.

Exactly, that is why it should be made private.

[kodebach]

But symbols in src/libs/elektra/symbols.map are still exported, i.e. it should only contain symbols that are used outside the library (.so) that defines them.

Symbols should be defined like this:

1. Used only within one file: static
2. Used within multiple files of a single library: non-static, but also not declared in a symbols.map
3. Used within multiple libraries internally, but not public API: declared in the appropriate symbols.map within the libelektraprivate_1.0 section
4. Public API: declared in the appropriate symbols.map in a section other than libelektraprivate_1.0

Note: "multiple libraries" in this context could also mean a single library and some tests, since tests are compiles as a separate ELF binary

This means, if ksSearchInternal is used in tests it should be in the private section. But if we rewrite the tests to use ksSearch, it should not be in the symbols.map at all (and could probably even be static).

[markus2330]

Yes 👍, ksSearchInternal can be static and then only ksSearch gets tested (which is a public symbol).