Page MenuHomePhabricator

[FileCheck] Simplify numeric variable interface
ClosedPublic

Authored by thopre on Jul 4 2019, 4:52 PM.

Details

Summary

This patch simplifies 2 aspects in the FileCheckNumericVariable code.

First, setValue() method is turned into a void function since being
called only on undefined variable is an invariant and is now asserted
rather than returned. This remove the assert from the callers.

Second, clearValue() method is also turned into a void function since
the only caller does not check its return value since it may be trying
to clear the value of variable that is already cleared without this
being noteworthy.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.Jul 4 2019, 4:52 PM
jhenderson added inline comments.Jul 5 2019, 1:49 AM
llvm/include/llvm/Support/FileCheck.h
74 ↗(On Diff #208098)

This phrasing feels a bit weird to me. How about:

"Sets value of this numeric variable, if undefined. The function will trigger an assertion failure if the variable is actually defined."

77 ↗(On Diff #208098)

irregardless -> regardless

llvm/lib/Support/FileCheck.cpp
28 ↗(On Diff #208098)

perhaps add "is not allowed".

thopre updated this revision to Diff 208150.Jul 5 2019, 4:58 AM
thopre marked 2 inline comments as done.

Address review comments

jhenderson added inline comments.Jul 5 2019, 5:39 AM
llvm/lib/Support/FileCheck.cpp
28 ↗(On Diff #208098)

This hasn't been addressed?

thopre updated this revision to Diff 208182.Jul 5 2019, 7:33 AM
thopre marked 3 inline comments as done.

Rebase on top of latest changes in patch series

thopre updated this revision to Diff 208183.Jul 5 2019, 7:34 AM

Address last comment

llvm/lib/Support/FileCheck.cpp
28 ↗(On Diff #208098)

Oops, dunno how I missed it.

This revision is now accepted and ready to land.Jul 5 2019, 7:54 AM
This revision was automatically updated to reflect the committed changes.

I was under the impression that an assertion in the public interface of a library API was not a great idea.

This revision is now accepted and ready to land.Jul 10 2019, 2:35 AM
This revision was automatically updated to reflect the committed changes.

I was under the impression that an assertion in the public interface of a library API was not a great idea.

Oops my bad, I forgot about that comment. I think only what's in the FileCheck class should be considered public API. The rest ought to be moved to a different header file and is private. FileCheckNumericVariable is an implementation details IMO.

I was under the impression that an assertion in the public interface of a library API was not a great idea.

Oops my bad, I forgot about that comment. I think only what's in the FileCheck class should be considered public API. The rest ought to be moved to a different header file and is private. FileCheckNumericVariable is an implementation details IMO.

OK that's fair, can be done later.