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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
