Details
- Reviewers
bcooksley - Maniphest Tasks
- T3689: Add abi compliance checker to CI
- Commits
- R857:1b6389ace5ca: Handle cases where tailing "/" in CMAKE_PREFIX_PATH fails the detection of…
Diff Detail
- Repository
- R857 CI System Tooling
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Aside from my comment above, all looks good to go in.
helpers/create-abi-dump.py | ||
---|---|---|
215 | Not sure why you're storing the list of prefix paths in prefixHeaders and then duplicating it here, unless you've plans to use prefixHeaders elsewhere? |
helpers/create-abi-dump.py | ||
---|---|---|
215 | Do you know the concept in python about mutable and unmutable datatypes? a set is mutable, that means if I do: a = set([1]) b = a b |= set([2]) print(b) {1, 2} # excepted print(a) {1, 2} # not really expected In this case we reuse`prefixHeaders` only once, so we wouldn't have to care about it. But still prefixHeaders would be lost. IMO this copy makes it more easy to extend the code, as you don't need to remember, that a set is mutable. noHeaders = set([os.path.abspath(i) for i in buildEnvironment.get('CMAKE_PREFIX_PATH').split(":")]) noHeaders |= set([os.path.join(i,"include") for i in noHeaders]) |
helpers/create-abi-dump.py | ||
---|---|---|
215 | I'm well aware of it, yes. I asked about that because it looked strange to see you prepare some data, then copy it to modify it again, while never referencing the original data again. If you are planning for the future, then that's fine, but it initially looked to me like it was a leftover from refactoring that had been done while writing this code. |