tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.
Couple of minor nits. LGTM otherwise.
================
Comment at: clang/include/clang/Basic/TargetID.h:42
+/// is not a null pointer.
+/// If \p CanonicalizeProc is true, canonicalize returned processor name.
+llvm::Optional<llvm::StringRef>
----------------
yaxunl wrote:
> tra wrote:
> > Comment needs updating as parameters and return value have changed.
> done
The comment still mentions `\p IsValid`.
================
Comment at: clang/include/clang/Basic/TargetID.h:56-58
+bool isValidTargetIDCombination(
+ const std::set<llvm::StringRef> &TargetIDs,
+ std::pair<llvm::StringRef, llvm::StringRef> *ConflictingTIDs = nullptr);
----------------
yaxunl wrote:
> tra wrote:
> > Looks like a good candidate for using a std::optional<std::pair> return
> > value.
> >
> done
`hasConflictingTargetIDCombination()` ? Optional is convertible to bool and
`has` better reflects the purpose of the function -- you want to know whether
there's a conflict. What exactly conflicts is sort of secondary info, only used
to provide additional details for diags.
================
Comment at: clang/lib/Basic/TargetID.cpp:161
+ if (llvm::any_of(Features, [&](auto &F) {
+ return ExistingFeatures.find(F.first()) == ExistingFeatures.end();
+ }))
----------------
Nit: `find(...) == end()` -> `count == 0` ? Makes it shorter and arguably
easier to read.
================
Comment at: clang/lib/Driver/Driver.cpp:2795-2799
+ auto CTID = getConflictTargetIDCombination(GpuArchs);
+ if (!CTID)
+ return true;
+ ConflictingTIDs = CTID.getValue();
+ return false;
----------------
Could be simplified a bit:
```
if (auto CTID = getConflictTargetIDCombination(GpuArchs)) {
ConflictingTIDs = CTID.getValue();
return false
}
return true;
```
Also, it does not seem to add any new functionality to
getConflictTargetIDCombination(). Perhaps it would make sense to change the
function signatures to match and just use `return
getConflictTargetIDCombination()`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60620/new/
https://reviews.llvm.org/D60620
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
