[Openmp-commits] [PATCH] D95912: [OpenMP] Attribute target diagnostics properly

[Mariya Podchishchaeva via Phabricator via Openmp-commits]

Fznamznon added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:1798
+  FunctionDecl *FD =
+      isa<FunctionDecl>(C) ? cast<FunctionDecl>(C) : dyn_cast<FunctionDecl>(D);
   auto CheckType = [&](QualType Ty) {
----------------
So, we assume that lexical context is a function and if it is not, we assume that the value declaration being checked is a function. Can we actually break this assumption?
For example, will something like this work correctly:
```
struct S {
    __float128 A = 1;
    int bar = 2 * A;
};
#pragma omp declare target
  S s;
#pragma omp end declare target
```


================
Comment at: clang/lib/Sema/Sema.cpp:1835
   }
+  if (const auto *FPTy = dyn_cast<FunctionNoProtoType>(Ty))
+    CheckType(FPTy->getReturnType());
----------------
I was a bit confused by `FPTy` variable name and already started writing a comment that we already checked a case `FunctionProtoType` case, then I realized that it is a check for Function*No*Prototype :) . Can we change FPTy to something closer to `FunctionNoProtoType` ?


================
Comment at: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp:42
 #ifndef _ARCH_PPC
-// expected-note at +1 {{'boo' defined here}}
+// expected-error at +2 {{'boo' requires 128 bit size '__float128' type support, but device 'nvptx64-unknown-unknown' does not support it}}
+// expected-note at +1 2{{'boo' defined here}}
----------------
So,  `boo` is diagnosed two times, at the point where it actually used and where it is defined, it looks a bit like a duplication isn't it? I think it makes sense to diagnose only usage point with note pointing to the definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95912