[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added inline comments.


================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:133
   /// returned if there are no more bundles to be read.
-  virtual StringRef ReadBundleStart(MemoryBuffer &Input) = 0;
+  virtual Expected<Optional<StringRef>>
+  ReadBundleStart(MemoryBuffer &Input) = 0;
----------------
ABataev wrote:
> sdmitriev wrote:
> > sdmitriev wrote:
> > > ABataev wrote:
> > > > Do we really need an inner `Optional` here?
> > > The idea was to return `StringRef` with bundle name when we have still have bundles and `None` when there are no more bundles in the file (BTW comment has to be updated to describe this). But I can restore the old convention where empty bundle name means 'no more bundles' in the file. In terms of functionality that would be the same, though use of `Optional<...>` makes intentions more explicit))
> > I have updated comment to describe the intended behavior.
> > @ABataev do you insist on removing inner `Optional<>` and restoring the current convention where empty string means the end of bundles in the file?
> The problem here is that `Expected` already handles the state and we have inner `Optional` for the same reason. Can we reuse `Expected` to indicate that there are no more bundles?
Well, `Expected` encodes two possible states (either Error or Value), but we need to encode three states - Error, Value or None. I do not have ideas how to do it without adding inner `Optional`. If you have, please share.


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

https://reviews.llvm.org/D67031