This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add include-mapping for C symbols.
ClosedPublic

Authored by hokein on Jun 13 2019, 7:42 AM.

Details

Summary

This resolves the issue of introducing c++-style includes for C files.

  • refactor the gen_std.py, make it reusable for parsing C symbols.
  • add a language mode to the mapping method to use different mapping for C and C++ files.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Jun 13 2019, 7:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 7:42 AM
kadircet added inline comments.Jun 13 2019, 10:54 AM
clang-tools-extra/clangd/include-mapping/cppreference_parser.py
1 ↗(On Diff #204539)

could we add a similar License and header comment section to this file as well ?

clang-tools-extra/clangd/include-mapping/gen_std.py
90 ↗(On Diff #204539)

maybe we should rather pass some something like "INVALID" as namespace for C symbols?

95 ↗(On Diff #204539)

I believe it is more sensible for this function to take (args.cppreference, args.language) and perform the above mentioned logic to generate parse_pages from these two internally.

clang-tools-extra/clangd/include-mapping/test.py
1 ↗(On Diff #204539)

no new tests for c symbol parsing ?

clang-tools-extra/clangd/index/CanonicalIncludes.cpp
97 ↗(On Diff #204539)

Let's drop the #NameSpace part from the expansion

clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
19 ↗(On Diff #204539)

could you also add some tests for C?

hokein updated this revision to Diff 204731.Jun 14 2019, 3:12 AM
hokein marked 7 inline comments as done.

Address review comments.

clang-tools-extra/clangd/include-mapping/gen_std.py
90 ↗(On Diff #204539)

changed to None

95 ↗(On Diff #204539)

Moving the above logic code to cppreference doesn't simplify the code, I'd prefer to keep the cppreference.py as simple as possible; and we also use the page_root after calling this function (line 99).

clang-tools-extra/clangd/include-mapping/test.py
1 ↗(On Diff #204539)

not needed, as we share the same parsing logic and we don't introduce new behavior in this patch.

kadircet accepted this revision.Jun 14 2019, 4:05 AM
kadircet added inline comments.
clang-tools-extra/clangd/include-mapping/gen_std.py
95 ↗(On Diff #204539)

i was referring to layering rather than simplicity. as for the accessing page_root below, it is to deduce cppreference version, which I believe should be handled by cppreference_parser again, but not that important.

This revision is now accepted and ready to land.Jun 14 2019, 4:05 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2019, 6:31 AM

This is awesome, but the script doesn't work anymore with -language=cpp -- is there any interest in looking into that? The C++ Standard Library has changed quite a bit recently and it would be great to update the mapping for the C++ Library.

Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2021, 12:53 PM
Herald added a subscriber: usaxena95. · View Herald Transcript