[PATCH] D92954: [clang-offload-bundler] Add option -list

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:176
+  /// List bundle IDs in \a Input.
+  virtual Error listBundleIDs(MemoryBuffer &Input) {
+    if (Error Err = ReadHeader(Input))
----------------
Now that listBundleIDs is only used by ` ListBundleIDsInFile()`, perhaps it could all be simplified and moved out of the class.


================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:882
+// List bundle IDs. Return true if an error was found.
+static Error ListBundleIDsInFile() {
+  // Open Input file.
----------------
I'd pass InputFileNames as an argument. Makes it easier to tell what the function needs.


================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:1031-1035
+      Error = true;
+      reportError(createStringError(
+          errc::invalid_argument,
+          "for the --outputs option: must be specified at least once!"));
+    }
----------------
Does it make sense to continue once we know that CLI options are wrong?
If we just early-exit with an error that may help simplifying the code below a bit.




================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:1047
+        errc::invalid_argument, "-unbundle and -list cannot be used together"));
+  } else if (ListBundleIDs) {
+    if (InputFileNames.size() != 1) {
----------------
Perhaps we can separate list option processing from bundle/unbundle?

I think if we could do something like this it would be more readable:
```
if (ListBundleIDs) {
  if (Unbundle) {
    error...
    exit.
  }
  ... other list-specific checks...
  ListBundleIDsInFile(InputFileNames)
  exit 0;
}

// complicated bundle/unbundle logic can proceed without having to bother about `list` option.
```


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

https://reviews.llvm.org/D92954