Page MenuHomePhabricator

[clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
Needs ReviewPublic

Authored by pfultz2 on Jul 12 2022, 8:59 AM.

Details

Summary

This check diagnoses instances where an enum is implicitly converted to an
integer. In C++11, enums can be defined as enum class which will prevent
such implicit conversion, however, enum provides no such guarantees to
prevent bugs. There can be many reasons why enum cannot be replaced with
enum class such as compatibility with C or legacy libraries.

This check will diagnose similar implicit conversions when using enum to
find the same class of bugs. Currently it will only warn on function or
constructor calls as such conversions are not clear to the user, but this
could be expanded in the future.

void foo(int i);
void f() {
    foo(e1); // e1 is implictly converted to an int
}

Diff Detail

Event Timeline

pfultz2 created this revision.Jul 12 2022, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 8:59 AM
pfultz2 requested review of this revision.Jul 12 2022, 8:59 AM
artem.tamazov requested changes to this revision.Jul 25 2022, 9:16 AM
artem.tamazov added a subscriber: artem.tamazov.

A typo in the doc, but otherwise LGTM.

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
9
This revision now requires changes to proceed.Jul 25 2022, 9:16 AM
pfultz2 updated this revision to Diff 451284.Aug 9 2022, 3:25 PM

Fix typo and merge from main.

pfultz2 marked an inline comment as done.Aug 9 2022, 3:26 PM
This revision is now accepted and ready to land.Aug 9 2022, 5:27 PM
pfultz2 updated this revision to Diff 451666.Aug 10 2022, 3:56 PM
pfultz2 retitled this revision from Add new clang-tidy check to find implicit conversions from enum to integer. to [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer..Aug 10 2022, 5:29 PM

Anymore feedback? Can this be merged now?

njames93 added a subscriber: njames93.

What is the motivation for requiring these to be the arguments of call or construct expressions?

Would be nice to add a note with an embedded fixit to silence this warning.
Something like this, though you may need to use the Lexer to get the correct location to insert the ).

auto Note = diag(MatchedExpr->getBeginLoc(), "insert an explicit cast to silence this warning", DiagnosticIDs::Note);
if (Result.Context->getLangOpts().CPlusPlus11)
  Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(), "static_cast<int>(")
           << FixItHint::CreateInsertion(MatchedExpr->getEndLoc().getLocWithOffset(1), ")");
else
  Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(), "(int)");
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
28

Remove this.

clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
18

FIXME

This revision now requires review to proceed.Aug 11 2022, 3:16 PM
njames93 added inline comments.Aug 11 2022, 3:18 PM
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
31

diagnostics in clang aren't formatted as full sentences.

What is the motivation for requiring these to be the arguments of call or construct expressions?

It is to just to try and limit the possible false positives at first. Usually a function call it is not clear locally that it will be converted to an integer but perhaps its common for users to assign an enum it an integer?

I could extend it to support everything except explicit casts if you think that would be better.

And for context, here is an actual bug this check would help find:

https://github.com/ROCmSoftwarePlatform/MIOpen/pull/1578#discussion_r889038610

I could extend it to support everything except explicit casts if you think that would be better.

If asked, I would vote for extending this warning.

pfultz2 updated this revision to Diff 453514.Aug 17 2022, 8:02 PM

Update review comments and added fixit hints.

pfultz2 marked 3 inline comments as done.Aug 17 2022, 8:03 PM
pfultz2 updated this revision to Diff 453515.Aug 17 2022, 8:05 PM
pfultz2 added reviewers: njames93, artem.tamazov.
pfultz2 updated this revision to Diff 454632.Aug 22 2022, 3:28 PM

Updated to diagnose return statements as well.

Anymore feedback?

So can this be merged now?

All review comments have been addressed. I assume this can be merged now.

@njames93 I fixed the review comments, can this be merged?