- User Since
- Aug 9 2020, 8:10 AM (16 w, 13 h)
Oct 21 2020
Oct 19 2020
No, I haven't checked over large C++ code bases, but I will do this.
Btw, one fear I have with this check is that the automated fixits are somewhat risky -- unscoped vs scoped enumerations has ABI implications and changing from one to the other may break the ABI.
I am not familiar with these specific ABI implications, if you could help me with some keywords/references/link to start to investigate, I am happy to deep dive into it. I am also willing to discard the automated fixes if it makes this review better. What do you think?
It is good to know, so it is not an issue.
- It doesn't take into account if an enum is defined in a header that is filtered out. Similarly to the previous one, I tried to find a solution for this, but I was unable (the ClangTidyCheck class can access the header filter information, but it doesn't expose it for the descendant classes). I also checked other checks, and they behave in the same way. Therefore I also think it is shouldn't be a blocking issue.
I recently pushed an upgrade to readability-identifier-naming where it would check the naming style for identifiers declared in header files, maybe thats something this could also use, this is the commit 4888c9ce97d8
It is not strongly connected to this review, but in the future I am planning to extend the check with:
- options to exclude enums, because changing them to scoped enumerations might not be suitable for every cases
Not strictly necessary, if people don't want the fix they could annotate the code with a // NOLINT(*prefer-unscoped-enums) comment.
I think with the inline suppression the users should annotate every usage of the enum while with the option it would be enough to list the enum's name to ignore it from the whole check. However it is just an improvement, when I reach that point I will do further digging about this topic, how the suppression actually work etc.
- options to force the doable fixes: based on my little experience it might be easier to force the doable fixes and manually fix the remaining ones
Forcing the fix is usually just a case of converting implicit cast usages of the constants into static casts.
What I meant is an option to express that the user don't care if there are some cases for an enum that cannot be fixed: fixed the other occurrences and the user will handle the rest. Currently the automated fix only fix the enums where every occurrence can be fixed. As a second thought this might be superfluous, because the other way is always possible: the user first fix the occurrences that cannot be fixed automatically and then use the check to fix the rest. So I think it is something that wouldn't mean real value.
Aug 24 2020
Fix clang-tidy issues.
I addressed all of the review comments. Apart from the small fixes I extended a checker logic.