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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:57
   support::endian::write32le(Data.data() + CRCPos, CRC32);
-  return Data;
+  return std::move(Data);
 }
----------------
avl wrote:
> grimar wrote:
> > avl wrote:
> > > grimar wrote:
> > > > You don't need this move I believe. Move constructor of `Expected<>` will be called anyways.
> > > It looks like in gcc case it does not work  - https://reviews.llvm.org/rG4da6927de47074f56531c2e7e2eecc4d6a1f09ec
> > Have you checked this place? The commit message does not say what was the BB error.
> > But in LLVM code we have many places that don't have `std::move` and feel fine.
> > 
> > https://github.com/llvm/llvm-project/blob/master/llvm/lib/Object/ArchiveWriter.cpp#L510
> > https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-exegesis/llvm-exegesis.cpp#L287
> > https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L582
> > 
> > Perhaps, the issue was related to `std::unique_ptr`.
> > 
> > 
> yes, the error was about unique_ptr:
> 
> MachOReader.cpp:337:10: error: could not convert ‘Obj’ from ‘std::unique_ptr<llvm::objcopy::macho::Object>’ to ‘llvm::Expected<std::unique_ptr<llvm::objcopy::macho::Object> >’
>    return Obj;
@grimar:

https://reviews.llvm.org/D43322

if there is a mismatch of the return type and the type of the local variable then std::move can be useful

if I am not mistaken, in fact, https://github.com/llvm/llvm-project/blob/master/llvm/lib/Object/ArchiveWriter.cpp#L510 is not a good example.


================
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");
----------------
grimar wrote:
> 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.
P.S. Doing quick grep of the repository shows that there is no consistency in naming Expected<..> variables. Certainly this is not ideal (I can see arguments for the both sides though personally don't have a very strong opinion on this). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88213