jdoerfert added inline comments.

================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2533
+    std::function<void(StringRef)> DiagUnknownTrait = [this, Loc](
+                                                        StringRef ISATrait) {};
+    TargetOMPContext OMPCtx(ASTContext, std::move(DiagUnknownTrait),
----------------
jdoerfert wrote:
> saiislam wrote:
> > jdoerfert wrote:
> > > Why doesn't this diagnose nothing?
> > Because an isa-feature will fail at least once, for either host compilation 
> > or device compilation. So, no point in always giving a warning.
> That is debatable. 
> 
> First, if I compile for a single architecture there is no device compilation 
> and it should warn.
> Second, if I place the metadirective into a declare variant function or add a 
> `kind(...)` selector to it it will also not warn even if you have multiple 
> architectures.
> 
> 
```
    ASTContext &Context = getASTContext();
    std::function<void(StringRef)> DiagUnknownTrait = [this,
    ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊                                CE](StringRef ISATrait) {
    ┊ // TODO Track the selector locations in a way that is accessible here to
    ┊ // improve the diagnostic location.
    ┊ Diag(CE->getBeginLoc(), diag::warn_unknown_declare_variant_isa_trait)
    ┊ ┊ ┊ << ISATrait;                                                   
    };
    TargetOMPContext OMPCtx(Context, std::move(DiagUnknownTrait),               
                                                                                
                                                                               
    ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊     getCurFunctionDecl(), 
DSAStack->getConstructTraits());
```
Already exists (SemaOpenMP). Why do we need a second, different diagnostic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116549

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to