Page MenuHomePhabricator

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

Authored by gbreynoo on Sep 11 2019, 8:12 AM.

Diff Detail

Event Timeline

gbreynoo created this revision.Sep 11 2019, 8:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2019, 8:12 AM

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. +@grimar @jhenderson in case they have suggestions.

llvm/test/tools/llvm-ar/mri-errors.test
9

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.

grimar added inline comments.Sep 14 2019, 5:11 AM
llvm/test/tools/llvm-ar/mri-errors.test
9

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 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. +@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?

llvm/test/tools/llvm-ar/mri-errors.test
1

Nit: white space -> whitespace

9

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.

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. +@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).

llvm/tools/llvm-ar/llvm-ar.cpp
1042

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);

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. +@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).

I am not sure though. The current approach is simple and I do not know/see for what else such handler can be usefull here. So I'd just go with this patch for now probably.

MaskRay added inline comments.Sep 17 2019, 8:28 AM
llvm/test/tools/llvm-ar/mri-errors.test
9

Just to make the action item concrete,

Test different ... -> ## Test different ...
RUN: -> # RUN:

gbreynoo updated this revision to Diff 220653.Sep 18 2019, 6:07 AM

Updated the test in line with suggested changes.

Thanks for all the feedback. Regarding an error handler I think the introduction of an error handler could be left for another time.

grimar accepted this revision.Sep 19 2019, 1:26 AM

LGTM with a nit.

llvm/tools/llvm-ar/llvm-ar.cpp
123–124

It is not clang-formatted.

This revision is now accepted and ready to land.Sep 19 2019, 1:26 AM
MaskRay accepted this revision.Sep 19 2019, 2:12 AM
This revision was automatically updated to reflect the committed changes.