Stringify the rest of the internal c++ macro API #3477
Conversation
The C version needs to construct a temporary string for find() anyway, but it's just tragic if we need to pass a C++ string as a const char * from which we then make another temporary string for this... No functional changes, but having the alternative around lets us convert call-sites one by one as it becomes relevant rather than making huge sweeping changes all at once.
The whole internal macro API needs this treatment, but at least now the expand*() functions take C++ strings. Except for the args of course, but that's a much bigger endeavour.
Options can't directly be replaced by a string because NULL and empty string have different meaning to the macro engine and will require some more elaborate solution. Leaving that to some other day.
|
Funny how most of those don't touch any code at all, but only the function headers. |
| @@ -1774,13 +1781,14 @@ static void pushMacroAny(rpmMacroContext mc, | |||
| } | |||
|
|
|||
| static void pushMacro(rpmMacroContext mc, | |||
| const char * n, const char * o, const char * b, int level, int flags) | |||
| const string & n, const char * o, const string & b, | |||
There was a problem hiding this comment.
I might be missing something, but using string on some places while std::string on other places seems inconsistent to me.
There was a problem hiding this comment.
It is, I noticed that too while doing this PR.
There's a kind of an inner logic in there: the functions that have a prototype in the header are using std::string in both places, other places take a the string shortcut. We should figure out a standard what to do with these and stick with that, but there are just sooooooo many such decisions to make, at a time when we're not really ready to make them 😅
There was a problem hiding this comment.
In my C++ days, I was never fond of using, because then you face this kind of dilemma.
There was a problem hiding this comment.
Yup. Typing the namespace everywhere seems tedious, but the alternative starts quickly looking worse.
Yup - because C++ will merrily automatically convert const char * to string, but not the other way around. |
The initial round left a lot of const char * arguments around, and trying to use this stuff, you bang your head into them sooner than later.
These are rather small and straightforward patches and could be probaby squashed into larger commits, but should be at least easy to review in this format.
One noteworthy exception (noted also in the commit message) is the parametric macro options that can't be converted to a string just like that because NULL and "" are two entirely different meanings there (NULL option is a non-parametric macro and "" is a parametric macro that takes no options)