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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
