[PATCH] D88213: [llvm-objcopy][NFC] refactor error handling. part 2.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:282
+    return createFileError(Config.InputFilename, O.takeError());
+  Object *Obj = O->get();
   assert(Obj && "Unable to deserialize COFF object");
----------------
mstorsjo wrote:
> avl wrote:
> > grimar wrote:
> > > avl wrote:
> > > > mstorsjo wrote:
> > > > > This bit seems like unrelated renamings?
> > > > During previous review it was asked to not to use such naming style for similar case - https://reviews.llvm.org/D88113#inline-818190 . So I changed it to match with that requirement.
> > > And `ObjOrErr` was probably a better name. It is common to name `Expected` variables as "<something>OrErr".
> > It was argued that such a naming match with  ErrorOr class. Since it is not the case here - it looks ambitions. https://reviews.llvm.org/D88113#inline-818190
> While these argument seem contradictory, the main point in this argument here, is that to implement what this patch sets out to do, you don't need to touch these lines at all, they already do exactly what they need to.
> 
> If you want to rename unrelated variables for consistency, that's a separate change.
> If you want to rename unrelated variables for consistency, that's a separate change.

Right.

> It was argued that such a naming match with ErrorOr class. Since it is not the case here - it looks ambitions.

Ok. I'd like to answer this. I disagree that it looks ambitions, because when you write something like:

```
Expected<ArrayRef<..>> Data = function();
```

The `Data` is not really data, but something that might contain either data or an error (`Expected::takeError()`).
In LLVMs code "OrErr" suffix is widely used and we don't have anything else, like "ExpectedData" or alike I think.

It is also not so rare to see the following:

```
Expected<ArrayRef<..>> DataOrErr = function();
if (!DataOrErr)
  return DataOrErr.takeError();
ArrayRef<..> Data = *DataOrErr;
```

Where it is obvious that `DataOrErr` and `Data` are completely different things.

But now I see that llvm-objcopy code is inconsistent and uses different styles when using `Expected<>`. And the rest of LLVM code is also not consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88213