Page MenuHomePhabricator

Add NamingStyle option to modernize-loop-convert.

Authored by angelgarcia on Sep 22 2015, 4:47 AM.

Diff Detail


Event Timeline

angelgarcia retitled this revision from to Add NamingStyle option to modernize-loop-convert..
angelgarcia updated this object.
angelgarcia added a reviewer: alexfh.
angelgarcia added subscribers: klimek, cfe-commits.

Remove related FIXME.

alexfh added inline comments.Sep 22 2015, 6:24 AM
815 ↗(On Diff #35362)

This can be a StringRef to avoid some copies.

820 ↗(On Diff #35362)

Once ContainerName is a StringRef, this can be replaced with

if (ContainerName.endswith("s"))
828 ↗(On Diff #35362)

The underscore here is not needed in the LLVM style.

832 ↗(On Diff #35362)

This should also handle LLVM style.

836 ↗(On Diff #35362)

What will that be? <container>_elem_elem? Doesn't make sense to me.

840 ↗(On Diff #35362)

The option with the trailing underscore doesn't make sense to me as well (unless we add a support for a style requiring trailing underscores for variable names).

844 ↗(On Diff #35362)

I'd go with give_me_name{0,1,2,...} or something else that would make it more obvious that the automatic name selection failed and a user input is needed.

418 ↗(On Diff #35362)

The IdentifierNamingCheck defines styles in a more generic way (CamelCase, camelBack, lower_case, UPPER_CASE). It seems reasonable to use a similar set of options. Maybe even move the enum from that check to a common place together with the conversion routines.

418 ↗(On Diff #35362)

Please add tests for all naming conventions.

448 ↗(On Diff #35362)

Please make it const and put it right after Context.

angelgarcia marked 10 inline comments as done.

Use CamelCase on the existing tests and add a test for every other naming convention.

alexfh edited edge metadata.Sep 23 2015, 4:59 PM

A few minor comments.

68 ↗(On Diff #35380)

The variable can be const, the one above as well.

820 ↗(On Diff #35380)

No need to check the length. endswith handles this itself.

841 ↗(On Diff #35380)

Automatic captures always look suspicious and require more attention. I'd avoid them unless there's a significant improvement in readability. I'd also prefer a more generic function, say AppendSuffixWithStyle, which could then be placed next to the styles enumeration and other functions to work with banking styles.

453 ↗(On Diff #35380)

nit: please remove the empty line.

angelgarcia edited edge metadata.

No need to check the length. endswith handles this itself.

I am checking that it is strictly greater than one, because we don't want an empty identifier after removing the "s".

alexfh accepted this revision.Sep 24 2015, 7:40 AM
alexfh edited edge metadata.

Looks good with a comment. Thank you!

48 ↗(On Diff #35608)

Please add a test for GIVE_ME_NAME_{0,1}. Same for other styles.

This revision is now accepted and ready to land.Sep 24 2015, 7:40 AM
angelgarcia edited edge metadata.


Still looks good.

Add nesting naming tests.

angelgarcia closed this revision.Sep 24 2015, 10:03 AM
This revision was automatically updated to reflect the committed changes.