Page MenuHomePhabricator

Improve error message of FileCheck when stdin is empty
ClosedPublic

Authored by davidb on Jan 31 2020, 9:54 AM.

Details

Summary

Replace '-' in the error message with <stdin>. This is also consistent with another error message in the code.

Diff Detail

Event Timeline

davidb created this revision.Jan 31 2020, 9:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2020, 9:54 AM
davidb marked an inline comment as done.
davidb added inline comments.
llvm/utils/FileCheck/FileCheck.cpp
619

I found it the least intrusive and confusing to change the InputFilename of stdin here, than in the other 3 places that the InputFilename is printed.

I have a vague and probably unreasonable concern that changing from the beahviour for a filename "<stdin>" could cause somebody some confusion somewhere, since it's non-standard. I do agree with changing the error message though (we do similar things already in other tools, IIRC). What are others' thoughts?

grimar added a comment.EditedFeb 3 2020, 1:55 AM

I have a vague and probably unreasonable concern that changing from the beahviour for a filename "<stdin>" could cause somebody some confusion somewhere, since it's non-standard.

FWIW on windows file names seems can't contain "<" or ">" characters. On ubuntu it seems to be OK.
But my non-very-strong-but-still-feeling here is that your concern is valid.

I do agree with changing the error message though (we do similar things already in other tools, IIRC).

Improving the error message sounds good to me too.

arichardson added inline comments.Feb 3 2020, 2:48 AM
llvm/utils/FileCheck/FileCheck.cpp
619

Instead of changing the option default to "<stdin>, may could you add:

if (InputFilename == "-")
  InputFilename = "<stdin>"; // Overwrite for improved diagnostic messages

Just after the the getFileOrSTDIN() call?

davidb updated this revision to Diff 242063.Feb 3 2020, 7:13 AM
  • review comments
davidb added a comment.Feb 3 2020, 7:22 AM

I have a vague and probably unreasonable concern that changing from the beahviour for a filename "<stdin>" could cause somebody some confusion somewhere, since it's non-standard. I do agree with changing the error message though (we do similar things already in other tools, IIRC). What are others' thoughts?

The initial change would never treat <stdin> as a file name, it would special case this on open and use the correct - value. We only try to open a file once, but use the filename in multiple places, hence this to me was the simplest way to achieve this.

I've updated the diff based on the review comments, which changes the name of the file after opening. I like this less as it changes the state, and in my opinion just as confusing, if not a bit more vulnerable... However I also recognise that the size of this file is small, it appears to be reasonably tested, and I am not likely to be making many more changes to this file in the immediate future, so I can tolerate this.

jhenderson accepted this revision.Feb 4 2020, 1:49 AM

This looks fine to me, although having looked at what other tools do, it might be better to create a reportError type of function that takes an input filename as an argument and does the conversion from '-' if needed. See for example llvm-readobj.cpp reportWarning etc. I'm happy either way, as a quick glance suggests this may not be straightforward here.

This revision is now accepted and ready to land.Feb 4 2020, 1:49 AM
This revision was automatically updated to reflect the committed changes.