rpmte: Move dependencies into an unordered_map #3538
Conversation
lib/rpmte.cc
Outdated
| RPMTAG_PROVIDENAME, RPMTAG_SUPPLEMENTNAME, RPMTAG_ENHANCENAME, | ||
| RPMTAG_REQUIRENAME, RPMTAG_RECOMMENDNAME, RPMTAG_SUGGESTNAME, | ||
| RPMTAG_CONFLICTNAME, RPMTAG_OBSOLETENAME, RPMTAG_ORDERNAME | ||
| }) { |
There was a problem hiding this comment.
Don't use code for what you can easily put into a data structure. Here's a nice opportunity to do just that.
lib/rpmte.cc
Outdated
| @@ -85,22 +77,22 @@ struct rpmte_s { | |||
| rpmfs fs; | |||
| }; | |||
|
|
|||
| /* Does not include RPMTAG_NAME as it needs special handling */ | |||
There was a problem hiding this comment.
The "special handling" is exactly what I wanted to see in the data structure, because then it's no longer "special" but something that follows a rule you can easily see in the structure.
There was a problem hiding this comment.
Just no. RPMTAG_NAME isn't a real dependency anyway. Bloating this datastructure with all kind of foo for a single case doesn't make any sense.
There was a problem hiding this comment.
No, the point is exactly that there IS a special case among these otherwise similar looking things, and the difference was far more obvious in the original code when all they're all next to each other. Looping the creation makes it actually worse for reading. Unless you put all the relevant into into a data structure where it's all again nicely next to each other.
It just usually happens that when you have one such special case, more accumulate. If you haven't noticed, this is a common pattern I'm asking for pretty much anywhere it comes up: prefer data structures over code.
There was a problem hiding this comment.
Well, I tried that until I realized how special the RPMTAG_NAME case really is. Moving it into the data structure seems kinda sensible to me. Adding flags looks pretty wrong in my eyes, but have a look yourself...
There was a problem hiding this comment.
Doing it with a magic flag certainly looks wrong, it does nothing but obfuscates. The way to put such a thing into a data structure is putting all the data into the structure, including functions to call (in this case the function signatures match) , eg:
{ { RPMTAG_NAME, RPMTAG_PROVIDENAME, RPMSENSE_EQUAL, rpmdsThisPool},
{ RPMTAG_PROVIDENAME, RPMTAG_PROVIDENAME, 0, rpmdsNewPool },
{ RPMTAG_REQUIRENAME, RPMTAG_REQUIRENAME, 0, rpmdsNewPool },
...
}
It looks dumb and repetitive but then it's just easy to follow data and then the code is really simple and free of any conditionals.
But shrug, I too missed the difference with rpmdsThisPool/rpmdsNewPool entirely, going back to the earlier version is fine - this really isn't worth all this time and energy spent by now.
There was a problem hiding this comment.
This is just typing out the code line by line with extra steps.
Chose the middle variant. At least it keeps the RPMTAG_NAME special case in one place.
There was a problem hiding this comment.
And yes, I missed the rpmdsThisPool thing, too. Even worse I thought is was about which pool to be used until I had a working patch ready...
There was a problem hiding this comment.
I find this version rather ugly, but enough is enough.
There was a problem hiding this comment.
So, "ugly" is not helpful or constructive in the slightest. Sorry about that. I also shouldn't have merged this.
We ended up with a version that's like a classic caricature compromise - the worst of both worlds: a one-purpose internal data structure that doesn't contain the data necessary to use it for initialization, that logic is far away inside a loop and none of that makes any sense at all. And, the internal data structure even leaks to the ABI.
When you got an idle moment, please submit a PR that changes this either to your original version with everything in the code logic, that was ten times saner than the final version. Or my suggestion where everything is in the data structure.
Somehow this PR went from ok to a mess during the review process with (to me at least) the worst version getting merged. There's bound to be some takeaways from this. One of them being simply avoid review and decisions when sleep deprived (which I was yesterday), but also other stuff. I need to think about this some.
Get rid of requires_ hack to avoid collision with requires keyword. Also the dependencies are looked up by tag anyway. No need for a case statements when we can use a container instead. Related: rpm-software-management#3537 Related: rpm-software-management#3518
Get rid of requires_ hack to avoid collision with requires keyword.
Also the dependencies are looked up by tag anyway. No need for a case statements when we can use a container instead.
Related: #3537
Related: #3518