[PATCH] D93068: [clang-offload-bundler] Add option -fail-on-missing-bundles

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 3 inline comments as done.
yaxunl added inline comments.


================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:980
+  if (FailOnMissingBundles && !Worklist.empty()) {
+    std::string ErrMsg = "Can't find bundles for";
+    std::set<StringRef> Sorted;
----------------
tra wrote:
> Do we need to report complete list of missing bundles?
> Can we just report the first one we've encountered and abort the command?
> 
Better to report all missing bundles since the info is available here and useful.


================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:985
+    unsigned I = 0;
+    unsigned Last = Worklist.size() - 1;
+    for (auto &E : Sorted) {
----------------
tra wrote:
> This assumes that all items on the `WorkList` were unique. 
> If some of the WorkList items were duplicated, then there will be fewer items in `Sorted` and the ` I == Last` comparison will never be true.
> I'd use `Sorted.size()` instead.
clang-offload-bundler does not allow duplicated targets. It asserts when duplicated targets in options are found when unbundling. Will add check for duplicate targets in options and emit error.


================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:998-999
+
   // If no bundles were found, assume the input file is the host bundle and
   // create empty files for the remaining targets.
   if (Worklist.size() == TargetNames.size()) {
----------------
tra wrote:
> Can we, instead use an explicit option to enable this 'make empty files for missing targets' and report missing targets as an error otherwise?
> I would assume that missing a target explicitly specified on command line would be an error more often than not. After all we've asked to extract something which implies that that 'something' exists. I.e. if I tell unzip to unpack a non-existing file from an archive, I do not expect it to give me an empty file.
will do


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

https://reviews.llvm.org/D93068