[PATCH] D142992: [include-mapping] Implement language separation in stdlib recognizer library

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:31
+
+static llvm::DenseMap<Lang, SymbolHeaderMapping *> LanguageMappings;
+
----------------
VitaNuo wrote:
> hokein wrote:
> > using a map here seems like an overkill, we have just 2 elements, I'd just use two separate variables (`CMapping`, and `CXXMapping`)
> what about the design idea that we might potentially want to extend this to multiple standards etc.? The idea is that it's extensible to `ObjC`, `OpenCL`... and so on and so forth, as has been discussed offline.
I think having a generic `Lang` enum structure is sufficient for the future extension, and I don't think we're going to add other languages in the foreseeable future (that's why I value the simple implementation at the beginning).

But you're right, getting the implementation right is probably a good idea. I'd like to remove the DenseMap, just use a raw array, something like below should work 

```
enum Lang {
   C = 0,
   CXX,

   LastValue = CXX,
};

// access by e.g. LanguageMappings[static_cast<unsigned>(Lang::C)].
static SymbolHeaderMapping* LanguageMappings[static_cast<unsigned>(Lang::LastValue) + 1];

```


================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:188
+  Lang RecognizerLang = Lang::CXX;
+  if (Language == clang::Language::C) {
+    RecognizerLang = Lang::C;
----------------
VitaNuo wrote:
> hokein wrote:
> > nit: just use `LangOpts.CPlusPlus` to check the language.
> There's `LangStandard::isCPlusPlus` method that I've just discovered. That's probably even better.
sorry if I was not clearer -- there is a bit `CPlusPlus` in the `LangOptions` which already does the equivalent thing (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/LangOptions.cpp#L107).

So we can just use  `if (D->getLangOpts().CPlusPlus)`.


================
Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:115
+
+  EXPECT_EQ(std::nullopt, stdlib::Symbol::named("", "int16_t"));
+  EXPECT_EQ(std::nullopt,
----------------
nit:  just `EXPECT_FALSE(stdlib::Symbol::named("", "int16_t")))`, following the existing pattern L46.


================
Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:122
+
+TEST(StdlibTest, LanguageSeparationForHeaders) {
+  EXPECT_NE(std::nullopt, stdlib::Header::named("vector"));
----------------
The existing `TEST(StdlibTest, All)` test seems like a good umbrella for the this test and the above test, how about moving them to the All test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142992