czhang added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35 + // This may work fine when optimization is enabled because bar() can + // be turned into a constant 7. But without optimization, it can + // cause problems. Therefore, we must err on the side of conservatism. ---------------- lebedev.ri wrote: > czhang wrote: > > aaron.ballman wrote: > > > czhang wrote: > > > > aaron.ballman wrote: > > > > > czhang wrote: > > > > > > aaron.ballman wrote: > > > > > > > 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. > > > > > > 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. > > > > > > Consider the case when synchronization is disabled for static > > > > > > initialization > > > > > > > > > > This is a compiler bug, though: > > > > > http://eel.is/c++draft/stmt.dcl#4.sentence-3 > > > > 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. > > > 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. > > 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. > > 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. > > I too want to see this explicitly spelled out in documentation. > > > Then they would like to check if the headers they didn't write may possibly > > run into this issue when compiling with this flag. > > I'm very much not a fan of this solution. > Are you sure that is not exposed in `LangOptions`, e.g. as > `ThreadsafeStatics`? > I'm very much not a fan of this solution. > Are you sure that is not exposed in LangOptions, e.g. as ThreadsafeStatics? 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62829/new/ https://reviews.llvm.org/D62829 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
