Page MenuHomePhabricator

[FileCheck] Introduce substitution subclasses
ClosedPublic

Authored by thopre on May 22 2019, 3:46 AM.

Details

Summary

With now a clear distinction between string and numeric substitutions,
this patch introduces separate classes to represent them with a parent
class implementing the common interface. Diagnostics in
printSubstitutions() are also adapted to not require knowing which
substitution is being looked at since it does not hinder clarity and
makes the implementation simpler.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.May 22 2019, 3:46 AM
jhenderson accepted this revision.May 22 2019, 6:42 AM

LGTM, with two small comments. Might be best getting @probinson to give a final thumbs up though, since he suggested it.

llvm/include/llvm/Support/FileCheck.h
141–143 ↗(On Diff #200691)

The explanations of the different substitutions performed (i.e. the second and third sentences of this comment) don't belong in this class here. I'd just remove them.

177–178 ↗(On Diff #200691)

Maybe better to rephrase this as "\returns a string containing the result...

This revision is now accepted and ready to land.May 22 2019, 6:42 AM
thopre updated this revision to Diff 200729.May 22 2019, 6:51 AM
thopre marked an inline comment as done.

Address review comments

thopre marked an inline comment as done.May 22 2019, 6:52 AM

Apart from missing 'override' keywords it looks pretty straightforward. LGTM.

llvm/include/llvm/Support/FileCheck.h
157 ↗(On Diff #200729)

This is virtual so should be marked 'override'.

161 ↗(On Diff #200729)

'override'

177 ↗(On Diff #200729)

override

181 ↗(On Diff #200729)

override

thopre updated this revision to Diff 200841.May 22 2019, 5:04 PM
thopre marked 4 inline comments as done.

Address review comments

This revision was automatically updated to reflect the committed changes.