[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.


================
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.
----------------
jfb wrote:
> 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.
You tempt me so with this comma operator... but no, I must not!


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:2051-2053
+      // FIXME this drops the error on the floor. This code is only used for
+      // typo correction and drops more than just this one source of errors
+      // (such as the directory creation failure above).
----------------
lhames wrote:
> This will crash if writeIndex ever generates an error. For that reason I would suggest writing this as:
> 
>   // FIXME: Can actually fail! <fixme goes here>
>   cantFail(GlobalModuleIndex::writeIndex(...));
> 
> It has the same effect, but without the control flow.
> 
I think this one shouldn't crash, so I've made it consume the error instead (and it still needs to get fixed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518