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:
> 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.
================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:100
+static int initialize() {
+ for (Lang Language : AllLangs)
+ initialize(Language);
----------------
hokein wrote:
> nit: just `for (Lang Language: {Lang::C, Lang::CXX})` or two statements
> `initilize(Lang::C);` and `initialize(Lang::CXX);`.
yes, same argument as above. I remember extensive discussions about the idea
that we might want to extend this to multiple language versions etc. in the
future.
================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:164
+ ensureInitialized();
+ SymbolHeaderMapping *Mapping = GetMapping(Language);
----------------
hokein wrote:
> Do we need the `ensureInitialized()` here? looks like no needed, we have
> called it in the Recognizer constructor,
you're right, not needed.
================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:188
+ Lang RecognizerLang = Lang::CXX;
+ if (Language == clang::Language::C) {
+ RecognizerLang = Lang::C;
----------------
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.
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
