Page MenuHomePhabricator

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

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



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


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
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.
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.
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
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
124 ↗(On Diff #66796)

Interesting, though it looks like there's now an official capital ẞ (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
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.


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
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.