Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

warning for expensive inline constructors #51202

Open Quuxplusone opened 2 years ago

Quuxplusone commented 2 years ago
Bugzilla Link PR52235
Status NEW
Importance P enhancement
Reported by Trass3r (trass3r@gmail.com)
Reported on 2021-10-20 09:18:11 -0700
Last modified on 2021-10-20 09:32:23 -0700
Version unspecified
Hardware Other All
CC blitzrakete@gmail.com, dblaikie@gmail.com, dgregor@apple.com, erik.pilkington@gmail.com, llvm-bugs@lists.llvm.org, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
(Please correct me if anything is wrong)

When ctors/dtors are defined inline they will be emitted into every object file
using them. It gets worse when debug info is enabled.
Then there's the potential issue of not being able to anchor the vtable in one
place and also the class type information.
https://blog.llvm.org/posts/2021-04-05-constructor-homing-for-debug-info/

Chrome already implemented such a warning as a plugin:
https://www.chromium.org/developers/coding-style/chromium-style-checker-errors#TOC-Constructor-Destructor-Errors
https://chromium.googlesource.com/chromium/src/tools/clang/+/refs/heads/main/plugins/FindBadConstructsConsumer.cpp

Not sure if it should be a warning or a clang-tidy check.
But it shouldn't be that expensive and would reach a larger audience as a
warning.
Quuxplusone commented 2 years ago

I /think/ this is still more of a stylistic (it's not a bug in every codebase - only some codebases) thing - admittedly we do have -Wweak-vtables, which is similarly esoteric and stylistic/build optimization, so there's some precedent. But generally we try not to add warnings that are off-by-default due to the fact that compiler warnings have a higher maintenance cost (due to the tight integration/being embedded in clang's code, rather than in a plugin/standalone module like clang-tidy).

Especially given the heuristic nature of what's "too complex" here, that probably also contributes to it being more suited to clang-tidy than to a clang warning.