hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land.
Thanks, this looks good to me! This requires some work to rebase for https://github.com/llvm/llvm-project/commit/e1aaa314a46cd303019da117bfd330611d5b7a84, I will rebase it and land it for you. ================ Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:31 + +static llvm::DenseMap<Lang, SymbolHeaderMapping *> LanguageMappings; + ---------------- VitaNuo wrote: > 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. yeah, it is the sad bit, but I think it is OK. 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
