2 comments:
File util/qualcomm/createxbl.py:
Patch Set #17, Line 673: if header_version == 5 or header_version == 6:
Explicitly checking for version 5 and 6 makes much sense, it is more user-friendly.
I'm trying to suggest code that will be easier to maintain going forward. Yes, there are only versions 5 and 6 right now, but there may be versions 7, 8 and 9 at some point in the future. In general, most development happens by adding new things on top of the latest version -- so it's more likely that version 7 will be the same as versions 5 and 6 in this aspect than version 3 and 4.
If you always write these things by explicitly checking for 5 and 6, that means when version 7 comes around and is mostly the same as 6 (with another minor change somewhere else) you will have to go around and find every single check like this in the code and update them to include 7. If you don't do that, it will break, and it makes it easy to miss the check somewhere (and if you're unlucky it might break in a subtle way that's not immediately obvious).
You shouldn't think of this as "do I want to specify this for version 7 already or not", because you *are* already specifying it for version 7 and later anyway. Whenever you write 'if version == 6: ... else: ...' you automatically say that version 7 and later will be the same as version 5 or below (because it will catch on the else: clause). I'm just saying that's unlikely. It's more likely that it will be similar to version 6. So I would write all these things as less-than/greater-than checks, it will most likely save you work in the future and it may save you from having to chase down weird bugs.
File util/qualcomm/mbn_tools.py:
Patch Set #17, Line 74: MBN_HEADER_VERSION_6 = 6 # Mbn header_version 6
It is always good to have constants which are pointing to numbers, even SHA256_SIGNATURE_SIZE is als […]
Well, for a SHA size it makes sense because the reader doesn't automatically know what the size is just from reading the name. But at the point where you are defining constants like
NUMBER_1 = 1
NUMBER_2 = 2
NUMBER_3 = 3
...
I'm not sure it really adds any value anymore.
Anyway, no big deal, I'm fine leaving this like this if you prefer it.
To view, visit change 35506. To unsubscribe, or for help writing mail filters, visit settings.