-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
ClangPlugins: Warn on Handle/SafeFunction members in GC allocated objects; Verify JS_CELL-like macros #24422
ClangPlugins: Warn on Handle/SafeFunction members in GC allocated objects; Verify JS_CELL-like macros #24422
Conversation
627f977
to
f310ee7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks very cool. I do wonder though, could we have 'nosiy' warnings behind a flag (or plugin opt..?) so that folks can enable the plugins for general libweb work without worrying about noise in their build logs?
Like a CMake flag that ends up adding Note to self: need to change the plugin names use underscores instead of dashes to be able to use |
f310ee7
to
d00ea71
Compare
This makes it more clear what it is used for
This allows arguments to be passed to them from the command line
GC-allocated objects should never have JS::SafeFunction/JS::Handle fields. For now the plugin only emits warnings here, as there are many cases of this occurring in the codebase that aren't trivial to fix. It is also behind a CMake flag since it is a _very_ loud warning.
d00ea71
to
54f4a0d
Compare
The plugins now detect if a GC-allocated class contains a field with a
Handle
/SafeFunction
type. Unfortunately there are many occurrences of this in the code base, so for now I've left it as a warning with a FIXME to change it to an error later. It is also very noisy at the moment, but that is because many warnings are emitted for each field occurrence. Not quite sure how to fix this, but in the mean time it can serve as motivation to change the field types 馃槂The plugins now also verify that the JS_CELL-like macros are used properly. This includes
JS_CELL
,JS_OBJECT
,JS_ENVIRONMENT
,JS_PROTOTYPE_OBJECT
, andWEB_PLATFORM_OBJECT
. For these macros, the first argument must exactly match the class name in the declaration, and the second argument must exactly match the parent name (namespaces included).Closes #23900, closes #23881, closes #23880