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

Mon Nov 23 08:18:57 PST 2020

Anastasia added a comment.

In D90928#2408249 <https://reviews.llvm.org/D90928#2408249>, @erik2020 wrote:

> In D90928#2405796 <https://reviews.llvm.org/D90928#2405796>, @Anastasia wrote:
>
>> Do you think we could improve testing? I presume there is something that triggers a failure without your change...
>
> I'm not really sure how to test this code. Best I can tell, there's no way for the `clang` executable to call these functions with invalid strings. I only ran into the seg faults when I was programmatically setting options using the clang API.

Oh, I see. We could also create a g-test for this but it's probably not worth enough. Perhaps if you just update a comment that the API behavior becomes clear it should be good enough.

================
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()) {
----------------
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`. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90928