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.
Details
- Reviewers
jdenny - Commits
- rGc347619bc2ba: [FileCheck] Fix naming of OverflowErrorStr var
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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. |
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? |
llvm/lib/FileCheck/FileCheck.cpp | ||
---|---|---|
124 | It seems fine. Thanks for clarifying. |
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.