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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 4 inline comments as done.
yaxunl 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))
----------------
tra wrote:
> Now that listBundleIDs is only used by ` ListBundleIDsInFile()`, perhaps it could all be simplified and moved out of the class.
listBundleIDsCallback needs to be a virtual function and it is called by listBundleIDs. Keep listBundleIDs as a member function together with listBundleIDsCallback shows their relation and is more readable.


================
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.
----------------
tra wrote:
> I'd pass InputFileNames as an argument. Makes it easier to tell what the function needs.
done


================
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!"));
+    }
----------------
tra wrote:
> 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.
> 
> 
done


================
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) {
----------------
tra wrote:
> 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.
> ```
done


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

https://reviews.llvm.org/D92954