[PATCH] D90928: [OpenCL] Check for extension string extension lookup

Mon Nov 23 11:03:05 PST 2020

Anastasia added inline comments.

================
Comment at: clang/include/clang/Basic/OpenCLOptions.h:47
   bool isSupported(llvm::StringRef Ext, const LangOptions &LO) const {
+    auto E = OptMap.find(Ext);
+    if (E == OptMap.end()) {
----------------
erik2020 wrote:
> Anastasia wrote:
> > Btw how about we use `isKnown` instead because it does exactly that? Also, I think we should update the comment to explain this change in the API behavior and add a comment for `isKnown`. 
> My thinking was that this way, there's only one look-up in `OptMap`, but when using `isKnown()` there are two. I don't know how good a compiler would be at inlining `isKnown()` and de-duplicating the look-ups.
> 
> But maybe the overhead doesn't really matter in this case and calling `isKnown()` is clearer?
I would say it is likely to be inlined in the optimized builds, removing duplicate calls to find would be trickier but since it's part of the standard libraries I think it is very likely going to happen.

Ok, if you are concerned about the performance we can leave it as is and only update the comments. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90928