This is an archive of the discontinued LLVM Phabricator instance.

Minor refactor of CanonicalIncludes::addSystemHeadersMapping.
ClosedPublic

Authored by ppluzhnikov on May 16 2022, 9:17 PM.

Details

Summary

Before commit b3a991df3cd6a SystemHeaderMap used to be a vector.

Commit b3a991df3cd6a changed it into a map, but neglected to remove
duplicate keys (e.g. "bits/typesizes.h", "include/stdint.h", etc.).

To prevent confusion, remove all duplicates, build HeaderMapping
one pair at a time and assert() that no duplicates are found.

Diff Detail

Event Timeline

ppluzhnikov created this revision.May 16 2022, 9:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 9:17 PM
ppluzhnikov requested review of this revision.May 16 2022, 9:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 9:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Sorry about the delay. Thanks for the cleanup! I only have a few minor comments here.

clang-tools-extra/clangd/index/CanonicalIncludes.cpp
762

Use size_t

764

The name per LLVM should be J per LLVM style guide. Also, why not I?

768

This will cause "unused" warning in builds with disabled assertions.
Add (void)Result; to avoid it.

768

Use assert(Result && "Duplicate in include mappings").

ppluzhnikov marked 4 inline comments as done.

Address review comments.

Thanks for the review.

Comments addressed in new revision.

ilya-biryukov accepted this revision.Jun 7 2022, 8:46 AM

LGTM. Thanks for the cleanup!

This revision is now accepted and ready to land.Jun 7 2022, 8:46 AM

Thanks for the review.

Could you commit this?
I don't have commit rights.

Could you commit this?

Sure! Done.