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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.





================
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;
----------------
Do we need to report complete list of missing bundles?
Can we just report the first one we've encountered and abort the command?



================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:985
+    unsigned I = 0;
+    unsigned Last = Worklist.size() - 1;
+    for (auto &E : Sorted) {
----------------
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.


================
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()) {
----------------
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.


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

https://reviews.llvm.org/D93068