Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Port Comment option flags to new parsing system

Authored by jansvoboda11 on Jul 13 2020, 10:11 AM.

Diff Detail

Event Timeline

dang created this revision.Jul 13 2020, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2020, 10:11 AM
jansvoboda11 commandeered this revision.Nov 18 2020, 4:04 AM
jansvoboda11 added a reviewer: dang.
jansvoboda11 added a subscriber: jansvoboda11.

Taking over this patch, as Daniel is no longer involved.

dexonsmith accepted this revision.Nov 18 2020, 11:26 AM

LGTM after you fix a couple of nits.


Nit: please end the comment with a period to make this a sentence; I'm not sure if "Options" is really a proper noun; I would have thought "Comment options.".


Might be nice to clean up this comment in a separate NFC commit ahead of time, to match what you do with "Comment Options".


Nit: is moving this option and adding a comment a necessary part of the patch, or is it just a cleanup that could be done before in an NFC commit? If it's possible, I would suggest moving it in a separate commit.

This revision is now accepted and ready to land.Nov 18 2020, 11:26 AM

Thanks. Committing this without the NFC change.


That's a fair point, moving the option is not necessary. I will revert the move in this patch and consider creating the // Comment options. section once all comment options get ported to the marshaling system.

This revision was landed with ongoing or failed builds.Nov 19 2020, 6:31 AM
This revision was automatically updated to reflect the committed changes.