This is an archive of the discontinued LLVM Phabricator instance.

MIR Parser: Simplify the token's string value handling.
ClosedPublic

Authored by arphaman on Aug 5 2015, 5:25 PM.

Details

Summary

This patch removes the StringOffset and HasStringValue fields from the MIToken struct and simplifies the stringValue method which now returns the new StringValue field.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 31422.Aug 5 2015, 5:25 PM
arphaman retitled this revision from to MIR Parser: Simplify the token's string value handling..
arphaman updated this object.
arphaman added a reviewer: silvas.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: llvm-commits.
silvas edited edge metadata.Aug 5 2015, 7:17 PM

Except for removing rawStringValue this entire suggestion was pretty nitpicky anyway, so feel free to ignore if any of this doesn't seem worth it.

lib/CodeGen/MIRParser/MILexer.h
110

It seems weird to me that we are adding an operator= (and a non-obvious one at that) for a class that is never really being assigned. It seems like the complexity is coming from the use of MIToken constructor to reinitialize an MIToken. I think it would be simpler to use two explicit calls:

  • resetToken(Kind, Range)

then one of

  • setIntVal(APSInt &)
  • setStrValNonOwned(StringRef) (or use an offset; but I like the symmetry with Owned)
  • setStrValOwned(StringRef/std::string) (copies into internal buffer and sets StrVal)

Conveniently all the existing uses of the MIToken constructors like this are already broken across two lines, so this should not increase the size of the reinitialization source code (but allows deleting code duplicated across these 3 constructors).

An even simpler solution is to just use std::string/SmallString and have it always own (not sure how performance-critical this is). Once it grows large enough there should be no further need to reallocate, so it is just a small memcpy cost.

arphaman updated this revision to Diff 31465.Aug 6 2015, 11:56 AM
arphaman edited edge metadata.

The updated patch now uses the 'reset' and 'set...' pattern instead of MIToken's constructors as suggested by Sean.

silvas added a comment.Aug 6 2015, 4:05 PM

Nice! I wasn't expecting a builder pattern but that turns out really nice! LGTM

silvas accepted this revision.Aug 6 2015, 4:06 PM
silvas edited edge metadata.

Mark as "Accept" in Phab. (don't think it really matters, but just to be consistent with the LGTM in the previous comment).

This revision is now accepted and ready to land.Aug 6 2015, 4:06 PM
This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.