Implement a native C++ macro API + use it to replace manual macro locking #3408
Conversation
This is all a such a hysterical mess but one of the preliminaries to be able to return C++ strings to callers is to have them in the first place. This doesn't make any real difference as such, just makes the result one step closer.
|
Got carried away enough that I forgot to explain the key benefits: And of course, you don't need to manually free those C++ strings these expands return, which is a death by thousand cuts with rpmExpand() and friends. These need to go to the main commit message of course, but I'll wait to see if there are other adjustments to make. |
|
The macro class needs a bit more doc strings. It is kinda weird that it is basically just a lock around the macro context. Not that there is anything wrong with that - it's just weird |
I don't disagree, but we don't traditionally have much in the way of docs for internal APIs. What do you want to see documented, the locking scheme?
This is basically how you want a locked resource to be handled in C++. It's a different paradigm from what we've had, sure. The various fooAcquire() things were a kind of emulation of this though. |
|
May be just add a two line comments before or just in the class declaration stating that this si for getting a lock on a macro context and use it to do macro stuff with it. |
Force all macro access through a C++ class which locks the context in constructor, convert all access to go through it and voila. This lets caller do multiple macro operations on one lock/unlock cycle, whereas the C API is forced to do one on each and every call. Generally the APIs with their C strings have been left alone, but the expand variants always return an actual result code, and take and return C++ strings to free callers, which is the other big benefit here. This looks big but is actually rather straightforward. New tests are not really needed because the C API internally now uses this API, and that gets thoroughly exercised.
|
Added a bunch of notes on the overall design + notes on API differences where they exist. |
|
This looks do to me now. May be @dmnks can have a look and second opinion before merging. |
|
The first commit seems to have forgotten to update the |
|
Oh, thanks for spotting, I'll fix it up. |
Ehm, I take that back, it's not fixed in the latest commit either, it still passes on a C string 😅 |
|
Right, I initially thought it was something else. This isn't a problem at all, C++ will merrily construct temporary string objects in a case like this. The other direction wont work though 😅 |
|
Ack, I wondered if that was the case, thanks 😄 |
This started with the innocent intention of making a couple of C++ string expand variants available, but then I got carried away...
There are no added tests because this is all covered by existing functionality testing - the last commit effectively adds a test for the expand variant accepting a vector.