[PATCH] D66011: [llvm-readobj] - Remove 'error(Error EC)' helper.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:385
 
 void reportError(StringRef Input, Error Err) {
+  if (!Err)
----------------
MaskRay wrote:
> grimar wrote:
> > MaskRay wrote:
> > > jhenderson wrote:
> > > > grimar wrote:
> > > > > MaskRay wrote:
> > > > > > reportError now checks `if (!Err)`. Is it still appropriate to call it `reportError`? I want to hear other opinions.
> > > > > Previously `error(Error EC)` did that. I can probably remove `if (!Err)` check to avoid the confusion.
> > > > I'd probably replace the `if (!Err)` check with an equivalent assertion. I think it's natural to write code like:
> > > > 
> > > > ```
> > > > if (Error E = doSomething())
> > > >   reportError(File, std::move(E));
> > > > ```
> > > > 
> > > > whereas the following looks weird to me:
> > > > 
> > > > ```
> > > > reportError(doSomethingWhichWillSucceed());
> > > > ```
> > > > 
> > > > An alternative name might be "checkError" if you didn't do that.
> > > `checkError` looks good to me.
> > I'd probably stick to `reportError` version. It looks simpler and more natural to use for me.
> I think I slightly prefer `Error` being the first argument. This allows to use default arguments for other fields if we supply more arguments, e.g.:
> 
> ```
> checkError(Error E, StringRef FileName, StringRef ArchiveName,
>              StringRef ArchitectureName = StringRef());
> ```
Ok, I made `Error` to be the first argument. I keeped `reportError` name and added an assert
to make sure it is used with `if`:

```
if (Err)
  reportError(Err, ...);
```


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

https://reviews.llvm.org/D66011