Page MenuHomePhabricator

[clang-tidy] Flag Classes Inheriting From Structs
Needs ReviewPublic

Authored by DennisMelamed on Sep 26 2018, 7:47 AM.

Details

Summary

Added a check which flags cases of a class inheriting from a struct, which can cause unintended scope issues since struct members are public by default.

Patch by Dennis Melamed

Diff Detail

Event Timeline

DennisMelamed created this revision.Sep 26 2018, 7:47 AM
DennisMelamed edited the summary of this revision. (Show Details)Sep 26 2018, 7:48 AM

Please upload patches with full context.
Please do follow coding style, clang-format your patches.

Would it make sense to not catch-all all the class : struct, but consider the explicitly specified inheritance visibility?

clang-tidy/misc/ClassInheritFromStructCheck.cpp
21

Please linewrap to 80 chars.

test/clang-tidy/misc-class-inherit-from-struct.cpp
13

Missing cases:

  • struct inheriting from struct
  • Different inheritance visibility:
    • You only check the default visibility
    • What if class explicitly-publicly inherits from struct?
    • What if class explicitly-privately inherits from struct?
    • What if class explicitly-protectedly inherits from struct?
    • Same for struct inheriting from struct?

This strikes me as likely being a very chatty diagnostic. For instance, it will trigger on harmless code that uses empty base class optimization (in addition to other concerns pointed out by @lebedev.ri). It seems like the real concern here is unintentional information disclosure, but I don't see a good heuristic for determining that something is "unintentional" or not. I'd be curious to know how frequently this triggers on some large C++ code bases.

docs/clang-tidy/checks/misc-class-inherit-from-struct.rst
6–10

I don't agree with the predicate here -- structs aren't just used for C compatibility (look at <type_traits> as an example). They're also useful when all members need to be public, which is exactly the situation you claim may be unintentional.

Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
109

Please use alphabetical order for new checks list.

docs/clang-tidy/checks/misc-class-inherit-from-struct.rst
6

Please make first statement same as in Release Notes.

15

I think warning is more correct term then error for Clang-tidy diagnistics.

lebedev.ri added inline comments.Sep 26 2018, 12:14 PM
test/clang-tidy/misc-class-inherit-from-struct.cpp
13

Also, i have only thought about it, there are no tests about what happens if you (say, publicly) inherit from struct, that does not have anything public.

Eugene.Zelenko added a project: Restricted Project.Sep 26 2018, 1:10 PM
JonasToth resigned from this revision.Dec 30 2019, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2019, 8:01 AM
Herald added a subscriber: mgehre. · View Herald Transcript