Handle cases where tailing "/" in CMAKE_PREFIX_PATH fails the detection of additional include directories.
ClosedPublic

Authored by knauss on Dec 14 2018, 4:32 PM.

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.
knauss requested review of this revision.Dec 14 2018, 4:32 PM
knauss created this revision.
bcooksley accepted this revision.Dec 14 2018, 6:18 PM

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?

This revision is now accepted and ready to land.Dec 14 2018, 6:18 PM
knauss added inline comments.Dec 14 2018, 7:33 PM
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.
If you want me to rewrite I would remove prefixHeaders completely and show by code, that the former set is lost:

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])
bcooksley added inline comments.Dec 16 2018, 9:09 AM
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.

knauss updated this revision to Diff 47791.Dec 18 2018, 7:16 PM

get rid of need to use copy.

This revision was automatically updated to reflect the committed changes.