This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Code factorization and cleanup in IdentifierNamingCheck
ClosedPublic

Authored by berenm on Sep 22 2015, 4:43 PM.

Details

Summary

This is to level the ground a little bit, in preparation for the changes in http://reviews.llvm.org/D13081.

Code factorization replaces all insertions to NamingCheckFailures map with a unique addUsage function that does the job.
There is also no more difference between the declaration and the references to a given identifier, both cases are treated as ranges in the Usage vector. There is also a check to avoid duplicated ranges to be inserted, which sometimes triggered erroneous replacements.

References can now also be added before the declaration of the identifier is actually found; this looks to be the case for example when a templated class uses its parameters to specialize its templated base class.

Diff Detail

Event Timeline

berenm updated this revision to Diff 35440.Sep 22 2015, 4:43 PM
berenm retitled this revision from to [clang-tidy] Code factorization and cleanup in IdentifierNamingCheck..
berenm updated this object.
berenm changed the visibility from "Public (No Login Required)" to "berenm (Beren Minor)".
berenm retitled this revision from [clang-tidy] Code factorization and cleanup in IdentifierNamingCheck. to [clang-tidy] Code factorization and cleanup in IdentifierNamingCheck.Sep 22 2015, 11:30 PM
berenm updated this object.Sep 22 2015, 11:32 PM
berenm added a reviewer: alexfh.
berenm changed the visibility from "berenm (Beren Minor)" to "Public (No Login Required)".
berenm added a subscriber: cfe-commits.
berenm added inline comments.Sep 23 2015, 12:49 AM
clang-tidy/readability/IdentifierNamingCheck.cpp
508

Hopefully the number of ranges in Usages will stay low. If not, Usages should be an hash table instead, but it looks like SourceRange isn't hashable as is.

alexfh edited edge metadata.Sep 24 2015, 9:44 AM

Please add more context to the diffs.

clang-tidy/readability/IdentifierNamingCheck.cpp
508

If the ranges represent unique variable usages, then there definitely will be corner cases that will make this code run sloooowly. I'd prefer this to be a llvm::SmallSet<>, llvm::DenseSet<>, std::unordered_set, std::set, std::multimap<SourceLocation, SourceLocation> or anything else providing a sub-linear lookup time. I'd probably go with llvm::SmallSet<> with a custom comparer for SourceRange.

berenm updated this revision to Diff 35651.EditedSep 24 2015, 10:32 AM
berenm edited edge metadata.

Changed Usages to RawUsageLocs, as DenseSet storing the raw encoding of SourceLocation instead of a vector or SourceRange.

berenm marked 2 inline comments as done.Sep 24 2015, 10:33 AM
berenm updated this revision to Diff 35669.Sep 24 2015, 1:00 PM

Reformatting with clang-format

alexfh accepted this revision.Sep 25 2015, 9:56 AM
alexfh edited edge metadata.

Looks good with a few comments.

Please tell me, if you need me to commit the patch for you after you address the comments.

clang-tidy/readability/IdentifierNamingCheck.cpp
574

This change assumes that the name is always represented by a single token. Please add a comment explaining why this can be done.

clang-tidy/readability/IdentifierNamingCheck.h
65

Please add a comment for this struct. Especially for the RawUsageLocs field.

73

80 character limit.

This revision is now accepted and ready to land.Sep 25 2015, 9:56 AM
berenm updated this revision to Diff 35818.Sep 27 2015, 1:22 AM
berenm edited edge metadata.

Added comments and reformated NamingCheckIdentifer.h with clang-format.

berenm marked 3 inline comments as done.Sep 27 2015, 1:24 AM

Please tell me, if you need me to commit the patch for you after you address the comments.

Yes please, I cannot commit it myself.

This revision was automatically updated to reflect the committed changes.
alexfh added inline comments.Sep 28 2015, 2:01 AM
clang-tidy/readability/IdentifierNamingCheck.cpp
574–576

I'd like the comment to cover destructors, operator names and other cases where the actual name consists of multiple tokens (which are probably filtered out somewhere, as we don't need to fix naming for these).

Fine for a follow-up.