jdoerfert added a comment.
Thanks for taking a look!
================
Comment at: clang/lib/Sema/Sema.cpp:1798
+ FunctionDecl *FD =
+ isa<FunctionDecl>(C) ? cast<FunctionDecl>(C) : dyn_cast<FunctionDecl>(D);
auto CheckType = [&](QualType Ty) {
----------------
Fznamznon wrote:
> 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
> ```
This is diagnosed neither with nor without the patch.
I'm not sure we should either because 2 * A is a constant expression.
We can make the example more complex using a constructor but I'd say that is
not related to this patch either way.
================
Comment at: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp:42
#ifndef _ARCH_PPC
-// expected-note@+1 {{'boo' defined here}}
+// expected-error@+2 {{'boo' requires 128 bit size '__float128' type support,
but device 'nvptx64-unknown-unknown' does not support it}}
+// expected-note@+1 2{{'boo' defined here}}
----------------
Fznamznon wrote:
> 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.
It is diagnosed twice, right. I don't think this is a problem of this patch
though, see for example line 54 on the left where we emit the boo error 4 times
before. Also, take a look at 181 and following in this file. With this patch we
reduce the number of times the error is emitted from 3 to 1. It's a bit of a
mixed bag but I think this makes it better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95912/new/
https://reviews.llvm.org/D95912
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
