This is an archive of the discontinued LLVM Phabricator instance.

Add NamingStyle option to modernize-loop-convert.
ClosedPublic

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
clang-tidy/modernize/LoopConvertUtils.cpp
815

This can be a StringRef to avoid some copies.

820

Once ContainerName is a StringRef, this can be replaced with

if (ContainerName.endswith("s"))
828

The underscore here is not needed in the LLVM style.

832

This should also handle LLVM style.

836

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

840

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

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.

clang-tidy/modernize/LoopConvertUtils.h
418

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

Please add tests for all naming conventions.

448

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.

clang-tidy/modernize/LoopConvertCheck.h
68

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

clang-tidy/modernize/LoopConvertUtils.cpp
821

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

830

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.

clang-tidy/modernize/LoopConvertUtils.h
447

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!

test/clang-tidy/modernize-loop-convert-uppercase.cpp
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.

Done!

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.