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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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?



================
Comment at: llvm/test/tools/llvm-ar/mri-errors.test:1
+Test different MRI comment formats and white space.
+
----------------
Nit: white space -> whitespace


================
Comment at: llvm/test/tools/llvm-ar/mri-errors.test:9
+RUN: not llvm-ar -M < %t/script1.mri 2>&1 | FileCheck --check-prefix=SCRIPT1 %s
+SCRIPT1: error: script line 2: Could not open library
+
----------------
grimar wrote:
> MaskRay wrote:
> > A bit inconvenience for you (sorry!): after D67558, the error messages are no longer capitalized, so this needs a rebase.
> > 
> > If I recall correctly, one point raised by @grimar or @jhenderson before is that we should still use `# ` comment markers even if the test contains no code. I hope they can confirm.
> Well, I also do not remember now, but I would be happy to see `##` as a comment marker everywhere in tests,
> or at least at places where there are no markers.
> @jhenderson, what do you think?
I have no strong preference for `#` versus `##`, as long as we use one in tests which don't necessarily need them, as it makes the comments stand-out from the rest of the test (same reason for using `##` in tests that need `#` for RUN lines etc). If someone else has a preference to use `##` everywhere, that's fine with me.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67449