[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via llvm-commits]

jfb marked 8 inline comments as done.
jfb added inline comments.


================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+      if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+        if (llvm::Error Skipped = Stream.SkipBlock())
+          return Skipped;
----------------
lhames wrote:
> Bigcheese wrote:
> > This is a pretty big behavior change.  Is clang-doc expecting to get files with unknown blocks?
> The inner test here is unsafe, as it will discard the outer error. You need:
> 
>   if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
>     if (llvm::Error Skipped = Stream.SkipBlock()) {
>       consumeError(std::move(Err));
>       return Skipped;
>     }
>     return Err;
>   }
> 
> or:
> 
>   if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
>     if (llvm::Error Skipped = Stream.SkipBlock()) {
>       return joinErrors(std::move(Err), std::move(Skipped));
>     return Err;
>   }
I don't think it expects this, and it looks (from the above code) like the intent is to handle errors when parsing (so this would be a bug that we'd want fixed).


================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+    Expected<unsigned> MaybeCode = Stream.ReadCode();
+    if (!MaybeCode) {
+      // FIXME this drops the error on the floor.
+      consumeError(MaybeCode.takeError());
+    }
+    
+    // FIXME check that the enum is in range.
----------------
lhames wrote:
> Bigcheese wrote:
> > Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.
> Yes: consumeError returns. This should probably be:
> 
>   if (!MaybeCode)
>     return consumeError(MaybeCode.takeError()), Cursor::Badness;
> 
Good point, fixed.


================
Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:489
 
   bool ReadBlockEnd() {
     if (BlockScope.empty()) return true;
----------------
thegameg wrote:
> Any reason why this doesn't return `Error`?
I'm not sure it's really an error: it goes to the end of the block either because it's empty, or because it pops the scope (which doesn't error either). It might be erroneously used, but I'm not sure we should make it an error right now. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518