[PATCH] D67449: [llvm-ar] Include a line number when failing to parse an MRI script

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D67449#1671048 <https://reviews.llvm.org/D67449#1671048>, @jhenderson wrote:

> In D67449#1670266 <https://reviews.llvm.org/D67449#1670266>, @MaskRay wrote:
>
> > I wish we can create a new function for MRI error reporting but as it stands, `runMRIScript` can call `readLibrary, addChildMember, and addMember` which can in turn call `fail` or `failIfError`, so `fail` has to be aware of the MRI line number. + at grimar @jhenderson in case they have suggestions.
>
>
> The only suggestion I have is to pass in an error handler from the calling code that can be used to generate an Error instance with a message customised to the caller. Default would be to do nothing special of course, but it would allow the MRI parser to pass one that refers back to a handler that appends the line number. That's potentially quite a significant refactor, so might be worth delaying to a subsequent change, but it might also be worth being done before this, so that we don't have to introduce the if statement in `fail`. What do people think?


I am not good familar with this tool's code yet, but at first look introducing a custom handler looks reasonable (that would help to remove most of these new variables it seems).



================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1042
     performOperation(ReplaceOrInsert, &NewMembers);
+  }
   exit(0);
----------------
Is there any reason to check `Saved`? Can it be just like the following?

```
  ParsingMRIScript = false;

  // Nothing to do if not saved.
  if (Saved)
    performOperation(ReplaceOrInsert, &NewMembers);
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67449