Finds instances where variables with static storage are initialized dynamically in header files.
Details
Diff Detail
Event Timeline
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
41 | Most of the matching should be done here, not in check(). |
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
38 | Will be good idea to address FIXME or remove if it's irrelevant. | |
55 | Will be good idea to address FIXME or remove if it's irrelevant. | |
56 | Could it be const auto *? | |
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst | ||
10 | Will be good idea to provide example. |
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
41 | I believe that there is no way to use the AST matching language to express hasConstantDeclaration. |
Hmm, but there already is clang's -Wglobal-constructors, that fires on some of these:
https://godbolt.org/z/rSnNuu
You are sure those extra warning this check produces ontop of -Wglobal-constructors are correct?
If so, maybe -Wglobal-constructors should be extended instead?
Yes, this change is for variables with static lifetimes that are not necessarily globally scoped. The name -global-constructors would be a misnomer. In addition, this check is intended purely for header files, for reasons documented in the .rst file. Although similar, I do not believe this check would make sense as an extension to -global-constructors.
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
40 | Do you want to restrict this matcher to only variable declarations that have initializers, or are you also intending for this check to cover cases like: // At file scope. struct S { S(); } s; | |
41 | You can write a local AST matcher to hoist this functionality into the check() call, though. Some of it is already exposed for you, like hasInitializer(), isValueDependent(), and isConstexpr(). | |
56 | We have an AST matcher for this (isExpansionInSystemHeader()). |
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
41 | Perhaps not done in the most elegant way, but there is some amount of non-trivial side effecting caused by checkInitIsICE that makes me a little wary try to chain this more declaratively. | |
56 | Seems like many of the google tidy checks choose to relegate this header checking (whence I copied this bit) in the checker, rather than match registration time. Not sure what the pros and cons are of hoisting, but I left it there to follow the convention of the other checks. |
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst | ||
---|---|---|
10 | Agreed, I'd like to see an example in the docs. | |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
34–36 | What problems can be caused here? Typically, dynamic init is only problematic when it happens before main() is executed (because of initialization order dependencies), but that doesn't apply to local statics. |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
---|---|---|
34–36 | Consider the case when synchronization is disabled for static initialization, and two threads call foo2 for the first time. It may be the case that they both try and initialize the static variable at the same time with different values (since the dynamic initializer may not be pure), creating a race condition. |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
---|---|---|
34–36 |
This is a compiler bug, though: http://eel.is/c++draft/stmt.dcl#4.sentence-3 |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
---|---|---|
34–36 | Sorry, I guess I didn't make it clear enough in the rst documentation file, but this check is for those who explicitly enable the -fno-threadsafe-statics flag because they provide their own synchronization. Then they would like to check if the headers they didn't write may possibly run into this issue when compiling with this flag. |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
---|---|---|
34–36 | Ah! Thank you for the explanation. In that case, this behavior makes more sense, but I think you should only warn if the user has enabled that feature flag rather than always warning. |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
---|---|---|
34–36 | I haven't been able to find much documentation on how to actually make a tidy check run against a feature flag. Is it possible to do this? I would think that said users would manually run this check on their header files. |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
---|---|---|
34–36 |
I too want to see this explicitly spelled out in documentation.
I'm very much not a fan of this solution. |
Be more explicit about -fno-threadsafe-statics
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
---|---|---|
34–36 |
Can you clarify what you mean? Are you not a fan of users disabling synchronization and providing their own? Or are you agreeing with Aaron that we should only enable this check with ThreadsafeStatics is enabled? Either way, I have put a check against LangOpts for this now. |
I think this last update clarified the concerns i raised in last comment.
But, did you mean to update the tests? They should have broke now that you require !getLangOpts().ThreadsafeStatics.
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst | ||
---|---|---|
12 | escape it: ``-fno-threadsafe-statics`` |
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
48 | Why is this check disabled for C++? I would expect dynamic init of a static in a C++ header file would be flagged by this check. | |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
2 | I'm a bit confused.
As best I can tell, this is a moral equivalent to doing: clang -x c whatever.h, and so this should be a compilation unit and not a header file. @alexfh, do you have thoughts there? |
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
48 | I'm confused now. If the language is not C++, we do an early return; that is, the check is run if we are on C++. Perhaps the early return is too confusing? |
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
48 | Then the question is opposite, is this meaningless for C? |
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp | ||
---|---|---|
48 | C can only initialize statics with constants, right? |
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp | ||
---|---|---|
2 |
This seems to have been cleared in a different comment: the check is C++-only.
Since the check is using a list of extensions to figure out, if a file is a header, it's not overly important to actually add a separate file that includes this one. And there's a certain benefit in convenience of not doing so (it's nice when CHECK-... and RUN: directives just work). At least one more check - misc-definitions-in-headers - does it the same way. |
Since Charlie has completed his internship before LGTM, I'll commit this on his behalf.
Will be good idea to address FIXME or remove if it's irrelevant.