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