[PATCH] D143160: [include-mapping] Introduce a human-edit CXXSymbolMapping file

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/include/clang/Tooling/Inclusions/CXXSymbolMap.inc:1
+//===-- CXXSymbolMap.inc ----------------------------------------*- C++ -*-===//
+//
----------------
kadircet wrote:
> also maybe rename this to, `AlternativeHeaderMap.inc` ?
Renamed it to `StdAlternativeHeaderMap.inc`, to align with the existing `StdSymbolMap.inc` name pattern for C++ symbols.


================
Comment at: clang/include/clang/Tooling/Inclusions/CXXSymbolMap.inc:12
+SYMBOL(consume_header, std::, <codecvt>) // declared
+SYMBOL(consume_header, std::, <locale>)  // defined
+SYMBOL(generate_header, std::, <codecvt>)
----------------
kadircet wrote:
> i feel like cppreference is wrong here (at least per libstdc++-12, and https://eel.is/c++draft/locale.syn#header:%3clocale%3e)
> 
> ```
> #include <locale>
> auto x = std::consume_header;
> ```
> 
> same for generate_header and little_endian.
> 
> it might be worth changing these into:
> ```
> SYMBOL(consume_header, std::, <codecvt>)
> // cppreference mentions this header as an alternative, but it isn't in practice.
> // SYMBOL(consume_header, std::, <locale>) 
> ```
Right, thanks for checking it! You're right. I also have checked the C++11, C++17 standard, I don't find any relevant text about the header "<locale>" contains the these symbol definition.
I'd rather delete this entry, and only keep `<codecvt>`. 



================
Comment at: clang/include/clang/Tooling/Inclusions/CXXSymbolMap.inc:19
+SYMBOL(mbstate_t, std::, <cuchar>)
+SYMBOL(size_t, std::, <cstddef>)
+SYMBOL(size_t, std::, <cstdio>)
----------------
kadircet wrote:
> can we actually use the ordering from standard: `[cstddef.syn], [cstdlib.syn], [cstring.syn], [cwchar.syn], [cuchar.syn], [ctime.syn], [cstdio.syn]` [(1)](https://eel.is/c++draft/libraryindex#:~:text=size_%C2%ADt%2C%20%5Bexpr.sizeof%5D%2C%20%5Bcstddef.syn%5D%2C%20%5Bcstdlib.syn%5D%2C%20%5Bcstring.syn%5D%2C%20%5Bcwchar.syn%5D%2C%20%5Bcuchar.syn%5D%2C%20%5Bctime.syn%5D%2C%20%5Bcstdio.syn%5D)
Sure. 


================
Comment at: clang/include/clang/Tooling/Inclusions/CXXSymbolMap.inc:26
+SYMBOL(size_t, std::, <cwchar>)
+// C++ [meta.trans.other 21.3.8.7]:
+// In addition to being available via inclusion of the <type_­traits> header,
----------------
kadircet wrote:
> nit: i don't think we need the comments here, both headers are also mentioned in cppreference pages.
> i am afraid these might get out of date (or only seldom mentioned) as future editors of this mapping might not be delicate enough.
Removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143160/new/

https://reviews.llvm.org/D143160