This is an archive of the discontinued LLVM Phabricator instance.

[TableGen][NFC] Group tokens with same attribute togather
ClosedPublic

Authored by wangpc on Jul 27 2023, 10:33 PM.

Details

Summary

So that we can simplify some code and simplify the implmentation
when we want to add new features (like adding new bang operators).

Diff Detail

Event Timeline

wangpc created this revision.Jul 27 2023, 10:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 10:33 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
wangpc requested review of this revision.Jul 27 2023, 10:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 10:33 PM
fpetrogalli added inline comments.Jul 31 2023, 3:54 AM
llvm/lib/TableGen/TGLexer.h
88

Why are Bit to TrueKW excluded from classification? If they cannot be clustered together, I think it would make sense to move them down to the bottom of the struct, so that that the closing of a group with *LAST would be followed directly by a *START of a new group.

wangpc updated this revision to Diff 545985.Aug 1 2023, 2:51 AM

Address comment.

This LGTM, thanks.

I have noticed that the "unclustered" values are still not grouped together. I thought it was a matter of just the few I mentioned in my first comment but apparently there are more.

I would have ordered the enum as { unclustered, group1, group2, ....}, just because it would have make it easier to find the cluster. But this is really debatable as it is my personal preference, no need to change it if you do not want to.

Thanks!

Francesco

llvm/lib/TableGen/TGLexer.h
157–158

same for these? Move them to the "unclustered" section?

165–166

same, move them to the unclustered section?

wangpc updated this revision to Diff 546036.Aug 1 2023, 6:56 AM

Address comment.

fpetrogalli accepted this revision.Aug 1 2023, 6:57 AM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 1 2023, 6:57 AM
wangpc marked 3 inline comments as done.Aug 1 2023, 6:57 AM

This LGTM, thanks.

I have noticed that the "unclustered" values are still not grouped together. I thought it was a matter of just the few I mentioned in my first comment but apparently there are more.

I would have ordered the enum as { unclustered, group1, group2, ....}, just because it would have make it easier to find the cluster. But this is really debatable as it is my personal preference, no need to change it if you do not want to.

Thanks!

Francesco

Yeah I agree with you!

This revision was landed with ongoing or failed builds.Aug 1 2023, 7:01 AM
This revision was automatically updated to reflect the committed changes.