This is an archive of the discontinued LLVM Phabricator instance.

[include-mapping] Add C-compatibility symbol entries.
ClosedPublic

Authored by hokein on Feb 2 2023, 1:32 PM.

Details

Summary

Extending the python generator:

  • to generate C-compatibility symbols
  • to generate macros

Diff Detail

Event Timeline

hokein created this revision.Feb 2 2023, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 1:32 PM
hokein requested review of this revision.Feb 2 2023, 1:32 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
kadircet added inline comments.Feb 6 2023, 1:53 AM
clang/tools/include-mapping/gen_std.py
270–271
279

what about having this logic as:

augmented_symbols = [symbol]
augmented_symbols.extend(GetCCompatibilitySymbols(symbol))
for s in augmented_symbols:
  ...

that way we can also encapsulate the rest of c-compat related logic into a more isolated place.

hokein updated this revision to Diff 495056.Feb 6 2023, 2:52 AM
hokein marked 2 inline comments as done.

address comments

kadircet added inline comments.Feb 6 2023, 4:30 AM
clang/tools/include-mapping/gen_std.py
122

nit: early exits

if header not in c_compat_headers:
  return []
if any(re.fullmatch(x, symbol.name) for x in exception_symbols)::
  return []
...
123

maybe add a comment like: Introduce two more entries, both in the global namespace, one using the C++-compat header and another using the C header.

126

i think it might be better to delete this line.

hokein updated this revision to Diff 495089.Feb 6 2023, 4:50 AM
hokein marked 3 inline comments as done.

address comments

kadircet accepted this revision.Feb 7 2023, 6:48 AM
This revision is now accepted and ready to land.Feb 7 2023, 6:48 AM
hokein updated this revision to Diff 495771.Feb 8 2023, 2:24 AM

update the standard-library tests, and fix a bug.

hokein updated this revision to Diff 495776.Feb 8 2023, 2:39 AM

fix the clangd unittest

Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2023, 2:39 AM
Herald added a subscriber: arphaman. · View Herald Transcript
hokein added inline comments.Feb 8 2023, 2:48 AM
clang-tools-extra/clangd/unittests/StdLibTests.cpp
37

This is a behavior change, I think it is probably fine. Would be nice to have a second look.

clang/unittests/Tooling/StandardLibraryTest.cpp
130

this patch exposed a regression caused by 1285172c21ef4867d9f895c0b2ab0f338c46e36f. The test was passed accidentally (both return a nullopt). This patch contains the fix (in StandardLibrary.cpp).

kadircet added inline comments.Feb 8 2023, 4:26 AM
clang-tools-extra/clangd/unittests/StdLibTests.cpp
37

let's change it to stdatomic.h instead.

clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp
220

nit: i think it's better to early-exit when D is nullptr rather than thinking about that in the rest of the logic.

clang/unittests/Tooling/StandardLibraryTest.cpp
67–68

as mentioned above, you can use stdatomic.h instead

67–68

maybe put everything below into a separate test for CCompat?

71–72

having a more comprehensive test here might be useful, e.g. check that std::int16_t only exists in C++ and just has <cstdint> as a provider. int16_t exists both in C and C++ with relevant providers

hokein updated this revision to Diff 495910.Feb 8 2023, 12:03 PM
hokein marked 3 inline comments as done.

address comments

clang-tools-extra/clangd/unittests/StdLibTests.cpp
37

stdatomic.h is still a c-compa header (though we don't have a entry in the StdSymbolMap.inc now). <iso646.h> might be better, it is empty file in C++ per C++ standard, so we never have a symbol for this header in C++ symbol map.

kadircet added inline comments.Feb 9 2023, 4:50 AM
clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp
221

oh i actually missed the behaviour change here in the previous version of the code and it seems very subtle.
can we change this function to take in a DeclContext* instead and terminate traversal inside the callsite at TUDecl and only treat TUDecl as global scope here?

hokein updated this revision to Diff 496102.Feb 9 2023, 5:44 AM

address comment

clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp
221

Modified this code per the suggestion, hope it is clearer now.

kadircet accepted this revision.Feb 9 2023, 6:24 AM

thanks, lgtm again

This revision was automatically updated to reflect the committed changes.