Page MenuHomePhabricator

[clang-tidy] misc-argument-comment non-strict mode
ClosedPublic

Authored by alexfh on Aug 3 2016, 2:05 PM.

Details

Summary

The misc-argument-comment check now ignores leading and trailing underscores and
case. The new StrictMode local/global option can be used to switch back to
strict checking.

Add getLocalOrGlobal version for integral types, minor cleanups.

Diff Detail

Repository
rL LLVM

Event Timeline

alexfh updated this revision to Diff 66712.Aug 3 2016, 2:05 PM
alexfh retitled this revision from to [clang-tidy] misc-argument-comment non-strict mode.
alexfh updated this object.
alexfh added a reviewer: hokein.
alexfh added a subscriber: cfe-commits.
hokein added inline comments.Aug 4 2016, 5:06 AM
clang-tidy/misc/ArgumentCommentCheck.cpp
29 ↗(On Diff #66712)

I think we should add a StringMode description in ArgumentCommentCheck's document.

180 ↗(On Diff #66712)

const auto *

188 ↗(On Diff #66712)

const auto *

aaron.ballman added inline comments.
clang-tidy/misc/ArgumentCommentCheck.cpp
124 ↗(On Diff #66712)

I think this is going to do the wrong thing for non-ASCII identifiers containing characters for which lowercase comparisons make no sense. I'm okay with that behavior (I don't think it should be a common occurrence), but would like to see a test demonstrating a failure case with a FIXME.

alexfh updated this revision to Diff 66796.Aug 4 2016, 6:22 AM
alexfh marked 2 inline comments as done.
  • Documented the StrictMode option, added a couple of consts.
alexfh marked an inline comment as done.Aug 4 2016, 6:22 AM
alexfh added inline comments.
clang-tidy/misc/ArgumentCommentCheck.cpp
124 ↗(On Diff #66712)

compare_lower will only modify ascii characters, all other characters will be compared as is.

As a purely theoretical question: are there characters that have lower-case variants, but it doesn't make sense to compare the lower-case variants? Can you give an example?

aaron.ballman added inline comments.Aug 4 2016, 6:48 AM
clang-tidy/misc/ArgumentCommentCheck.cpp
124 ↗(On Diff #66796)

Correct, which means this won't behave properly in some locales with UTF-8 identifiers. Consider Turkish, where İ (U+0130 “Latin Capital Letter I With Dot Above”) is the uppercase form of ı (U+0131 “Latin Small Letter Dotless I”). If the comment contains one version while the identifier contains the other, the comparison will currently fail, while a locale-aware comparison would succeed. You run into similar things with SS vs ß in German as well, where the uppercase form is two characters while the lowercase is only a single character.

alexfh added inline comments.Aug 4 2016, 7:22 AM
clang-tidy/misc/ArgumentCommentCheck.cpp
124 ↗(On Diff #66796)

Interesting, though it looks like there's now an official capital ẞ https://en.wikipedia.org/wiki/Capital_%E1%BA%9E (which is not frequently needed anyway, I guess).

At the end of the day, what we get is that the non-strict mode is currently somewhat stricter for non-ascii characters. Similar will happen with all other parts in LLVM that rely on StringRef::compare_lower. I don't think we need a separate test for this _here_, since it's a problem on a completely different level. And I guess the use non-ascii identifiers in C++ will cause much more serious problems than a slightly stricter clang-tidy warning ;]

aaron.ballman added inline comments.Aug 4 2016, 7:27 AM
clang-tidy/misc/ArgumentCommentCheck.cpp
124 ↗(On Diff #66796)

We may just have different testing philosophies -- I would advocate for a test because we know of a use case that's broken with this particular use of compare_lower. Not all uses of compare_lower are problematic, after all. However, I'm not going to fight for that test case too hard because this is hopefully an edge case that is low-impact. A FIXME would also suffice.

alexfh updated this revision to Diff 66806.Aug 4 2016, 7:43 AM
  • Added a FIXME.
aaron.ballman accepted this revision.Aug 4 2016, 7:45 AM
aaron.ballman added a reviewer: aaron.ballman.

LGTM!

This revision is now accepted and ready to land.Aug 4 2016, 7:45 AM
alexfh added inline comments.Aug 4 2016, 7:45 AM
clang-tidy/misc/ArgumentCommentCheck.cpp
124 ↗(On Diff #66796)

I'm reluctant to add a case, since the cost of making it work and maintaining on both linux and windows is higher than the value of it, IMO (it's my take out from writing clang-format's limited support for Unicode).

alexfh added a comment.Aug 4 2016, 8:01 AM

hokein: I'm committing the patch; if you have more comments, I'm happy to address them in a follow-up.

This revision was automatically updated to reflect the committed changes.