Convert main data structures in elfdeps to C++ #3581
Conversation
| return 1; | ||
|
|
||
| if (rstreqn(soname, "ld.", 3) || rstreqn(soname, "ld-", 3) || | ||
| rstreqn(soname, "ld64.", 3) || rstreqn(soname, "ld64-", 3)) |
There was a problem hiding this comment.
Heh, there seems to have been a bug here (comparing at most 3 chars whereas the tested strings are longer). That's now fixed with the conversion 😄 Nice.
There was a problem hiding this comment.
Oh, look at that. I didn't notice... Funny timing as we just talked about humans and calculating string lengths yesterday 😆
That bug exists in many 4.x releases then.
There was a problem hiding this comment.
Indeed, I've filed #3596 to cover the backports.
tools/elfdeps.cc
Outdated
| const std::string & soname, | ||
| const std::string & ver, const std::string & marker) | ||
| { | ||
| if (skipSoname(soname.c_str())) |
There was a problem hiding this comment.
This was probably forgotten as part of the "Use a C++ string for skipSoname() convenience" commit, i.e. the c_str() should be removed 😅
There was a problem hiding this comment.
Oh, now I somewhat recall that you can actually assign a NULL-terminated string to an std::string variable. Still, it seems like the conversion can be removed for clarity anyway.
There was a problem hiding this comment.
Oh yup, that's just an accidental leftove caused by "wrong" refactoring order - reordered the two relevant commits and voila the issue goes away 😅 Thanks for spotting.
|
Other than the |
Do away with the silly typedef while at it.
Rename addDep() to addSoDep() to hint at the intended use, and add a bare version of addDep() that has no extra logic. It's a thin layer of isolation but isolation nevertheless. No functional changes.
Allegedly no functional changes.
Don't bother checking for non-empty soname in the callers, addSoDep() will check for this anyhow.
The marker and ver don't *need* to be STL strings, but life is simpler if they all follow the same conventions. Just pass empty strings in place of NULL now and adjust the logic accordingly.
pmatilai
force-pushed
the
cxx-elfdeps
branch
from
eb179cd to
7c472a9
Compare
I ended up doing this to make dealing with #2197 saner. The multi-arch stuff itself is not ready for merging but moving to STL stuff is something we'll want to do anyway going forward, regardless of what/when happens with the multi-arch stuff, and this'll be easier to review separately anyhow.
Not adding any new tests here because elfdeps is reasonably covered already (as in, made itself known while working on this 😅 )