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