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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:31
+
+static llvm::DenseMap<Lang, SymbolHeaderMapping *> LanguageMappings;
+
----------------
hokein wrote:
> 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];
> 
> ```
Ok, this will need some casting, but I don't have a strong opinion.


================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:188
+  Lang RecognizerLang = Lang::CXX;
+  if (Language == clang::Language::C) {
+    RecognizerLang = Lang::C;
----------------
hokein wrote:
> 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)`.
Ah ok, thanks.


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


================
Comment at: clang/unittests/Tooling/StandardLibraryTest.cpp:122
+
+TEST(StdlibTest, LanguageSeparationForHeaders) {
+  EXPECT_NE(std::nullopt, stdlib::Header::named("vector"));
----------------
hokein wrote:
> 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?
Ok, I've moved everything to existing test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142992