Page MenuHomePhabricator

Misplaced const-qualification with typedef types
ClosedPublic

Authored by aaron.ballman on Jun 6 2016, 12:45 PM.

Details

Summary

Sometimes it's easy for developers to not notice that the type they are working with is a typedef type to a pointer and add a const-qualifier that applies to the typedef type rather than the underlying pointer type. e.g.,

typedef int *int_ptr;
void func(const int_ptr ip);

This results in ip having the type int * const rather than const int *.

This patch adds a new clang-tidy check to diagnose such constructs to alert the developer of the difference in types. It does not diagnose if the underlying pointer type points to a const type or is a function type because the const-qualifier on the typedef is more likely to be intentional.

Diff Detail

Event Timeline

aaron.ballman retitled this revision from to Misplaced const-qualification with typedef types.
aaron.ballman updated this object.
aaron.ballman added reviewers: alexfh, sbenza, hokein.
aaron.ballman added a subscriber: cfe-commits.
sbenza edited edge metadata.Jun 6 2016, 1:03 PM

I think this would be more interesting with macros.
Eg triggering in code like this:

#define FOO(type, op) const type& X = op()
FOO(int*, bar);
clang-tidy/misc/MisplacedConstCheck.cpp
33

guessAlternateQualification (lower case)

test/clang-tidy/misc-misplaced-const.c
21

If we are guessing that 'const' goes to the int, shouldn't we guess that volatile also goes to the int?

21

Only keep the first instance with the full message.
The rest can be collapsed with {{.*}} to try to fit in 80 cols.

test/clang-tidy/misc-misplaced-const.cpp
21

I assume this check does not trigger for 'const X&' where X is a pointer.
Please add a test for that.

aaron.ballman marked an inline comment as done.Jun 6 2016, 1:18 PM

Thank you for the quick turn-around!

test/clang-tidy/misc-misplaced-const.c
21

Actually, that test was purposely showing that we do *not* treat all cv-qualifiers the same. volatile and restrict have sufficiently complex semantics that I would be wary about pointing out the type differences as being potentially problematic. Typically, if you are using those qualifiers, you probably know more about what you want the code to do and whether you want the *pointer* to have the qualifier instead of the pointee.

21

I'll collapse the ones after the volatile test.

test/clang-tidy/misc-misplaced-const.cpp
21

Do you want:

template <typename Ty>
struct S {
  const Ty *i;
  const Ty &i2;
};

template struct S<int>;
template struct S<ip>; // ok
template struct S<cip>;
template struct S<int *>; // ok

Or something else?

hokein added inline comments.Jun 7 2016, 2:32 AM
test/clang-tidy/misc-misplaced-const.c
18

Will the check show the warning on ip const i3 = 0;? Technically, const ip i3 = 0 is exactly the same with ip const i3 = 0;.

aaron.ballman marked an inline comment as done.Jun 7 2016, 5:03 AM
aaron.ballman added inline comments.
test/clang-tidy/misc-misplaced-const.c
18

Yes, it will, but I should add a test case for it just the same.

aaron.ballman edited edge metadata.
aaron.ballman marked an inline comment as done.

Updated based on review feedback:

  • Added another two test cases (one for templates and one for const binding to the right of the typedef type).
  • Updated test cases to be less verbose in message checking
  • Corrected case for a static function
aaron.ballman marked 2 inline comments as done.Jun 7 2016, 5:13 AM

I think this would be more interesting with macros.
Eg triggering in code like this:

#define FOO(type, op) const type& X = op()
FOO(int*, bar);

Sorry, I originally missed this comment.

That might be a reasonable addition to this check, but strikes me as a future enhancement. Are you looking for the patch to cover this case initially, or is it fine for a follow-up?

sbenza added a comment.Jun 7 2016, 9:58 AM

I think this would be more interesting with macros.
Eg triggering in code like this:

#define FOO(type, op) const type& X = op()
FOO(int*, bar);

Sorry, I originally missed this comment.

That might be a reasonable addition to this check, but strikes me as a future enhancement. Are you looking for the patch to cover this case initially, or is it fine for a follow-up?

Yes, a follow up is ok. I was just making sure this is considered.

sbenza added inline comments.Jun 7 2016, 10:01 AM
clang-tidy/misc/MisplacedConstCheck.cpp
23

allOf() is unnecessary

27

Should we ignore pointer to members?
If not, we should add a test for those.

aaron.ballman marked 2 inline comments as done.

Corrected more review feedback.

clang-tidy/misc/MisplacedConstCheck.cpp
23

Good catch, that was a holdover from when I was using clang-query.

27

Pointer to members are not pointer types (they're a MemberPointerType which derives from Type, not PointerType), so they are ignored already. I think that's reasonable because we'd want the pointer to member function types to behave the same as function types. I'll add an explicit test case.

sbenza accepted this revision.Jun 7 2016, 10:21 AM
sbenza edited edge metadata.
sbenza added inline comments.
clang-tidy/misc/MisplacedConstCheck.cpp
23

You shouldn't need it for clang-query either.

This revision is now accepted and ready to land.Jun 7 2016, 10:21 AM
aaron.ballman added inline comments.Jun 7 2016, 10:25 AM
clang-tidy/misc/MisplacedConstCheck.cpp
23

Oddly enough, I did. What's more, there's a bug with hasType that I didn't have time to track down. It seems hasType(allOf(...)) always fails but allOf(hasType(...), hasType(...)) works. :-(

aaron.ballman closed this revision.Jun 7 2016, 10:29 AM

Committed in r272025.

hokein edited edge metadata.Jun 7 2016, 12:06 PM

@aaron.ballman, you forgot to add the check in docs/ReleaseNotes.rst.