This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Fix naming of OverflowErrorStr var
ClosedPublic

Authored by thopre on Mar 10 2021, 6:15 AM.

Details

Summary

As pointed out by Joel E. Denny in D97845, the OverflowErrorStr variable
is misnamed because the error is raised for any parsing error. Note that
in FileCheck proper this only happens in case of (under|over)flow
because the regex will ensure a number in the correct format is matched.

Diff Detail

Event Timeline

thopre created this revision.Mar 10 2021, 6:15 AM
thopre requested review of this revision.Mar 10 2021, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 6:15 AM

Other than a comment request, LGTM.

llvm/lib/FileCheck/FileCheck.cpp
124

Could you add a comment somewhere in this function explaining the significance for the FileCheck utility vs. the library? For me, this point has been a source of confusion.

jdenny accepted this revision.Mar 10 2021, 1:25 PM
This revision is now accepted and ready to land.Mar 10 2021, 1:25 PM
thopre updated this revision to Diff 329772.Mar 10 2021, 2:20 PM
thopre marked an inline comment as done.

Add comment to explain why the error message does not mention underflow/overflow

llvm/lib/FileCheck/FileCheck.cpp
124

Actually this method is only called through the checkInput API of the FileCheck library. The Pattern object is not visible and thus the regex matched is one produced by FileCheck. So even users of the library should not be able to get anything else than an overflow error. I think I purposedly made the error message vague enough to account for potential future use of this method (i.e. the method is more self contained).

I think it would be better to distinguish between overall behavior (only overflow happen) Vs how the method could be called (invalid StrVal). What do you think of the proposed comment?

jdenny added inline comments.Mar 10 2021, 4:52 PM
llvm/lib/FileCheck/FileCheck.cpp
124

It seems fine. Thanks for clarifying.

This revision was landed with ongoing or failed builds.Mar 11 2021, 2:30 AM
This revision was automatically updated to reflect the committed changes.