[PATCH] D143214: [include-mapping] Add C-compatibility symbol entries.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/StdLibTests.cpp:37
   EXPECT_THAT(CXX, HasSubstr("#include <cstdio>"));
-  EXPECT_THAT(CXX, Not(HasSubstr("#include <stdio.h>")));
+  EXPECT_THAT(CXX, HasSubstr("#include <stdio.h>"));
 
----------------
hokein wrote:
> This is a behavior change, I think it is probably fine. Would be nice to have a second look.
let's change it to `stdatomic.h` instead.


================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:217
+NSSymbolMap *Recognizer::namespaceSymbols(const NamespaceDecl *D, Lang L) {
+  // D could be nullptr!
   auto It = NamespaceCache.find(D);
----------------
nit: i think it's better to early-exit when D is nullptr rather than thinking about that in the rest of the logic.


================
Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:68
   EXPECT_THAT(stdlib::Symbol::all(), Contains(*Vector));
-  EXPECT_FALSE(stdlib::Header::named("<stdint.h>"));
-  EXPECT_FALSE(stdlib::Header::named("<stdint.h>", stdlib::Lang::CXX));
+  EXPECT_TRUE(stdlib::Header::named("<stdint.h>", stdlib::Lang::CXX));
   EXPECT_TRUE(stdlib::Header::named("<stdint.h>", stdlib::Lang::C));
----------------
as mentioned above, you can use `stdatomic.h` instead


================
Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:68
   EXPECT_THAT(stdlib::Symbol::all(), Contains(*Vector));
-  EXPECT_FALSE(stdlib::Header::named("<stdint.h>"));
-  EXPECT_FALSE(stdlib::Header::named("<stdint.h>", stdlib::Lang::CXX));
+  EXPECT_TRUE(stdlib::Header::named("<stdint.h>", stdlib::Lang::CXX));
   EXPECT_TRUE(stdlib::Header::named("<stdint.h>", stdlib::Lang::C));
----------------
kadircet wrote:
> as mentioned above, you can use `stdatomic.h` instead
maybe put everything below into a separate test for CCompat?


================
Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:71
 
-  EXPECT_FALSE(stdlib::Symbol::named("", "int16_t"));
-  EXPECT_FALSE(stdlib::Symbol::named("", "int16_t", stdlib::Lang::CXX));
+  EXPECT_TRUE(stdlib::Symbol::named("", "int16_t", stdlib::Lang::CXX));
   EXPECT_TRUE(stdlib::Symbol::named("", "int16_t", stdlib::Lang::C));
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143214